support for reverse proxy HTTP-Header based authentication #98

MichaelSp · 2019-09-02T13:05:19Z

This adds a feature similar to what https://github.com/jenkinsci/reverse-proxy-auth-plugin already implements. I couldn't use that plugin however, because of https://issues.jenkins-ci.org/browse/JENKINS-29330
I turned out to be much simpler to add the reverse-proxy authentication into this plugin than to add the recursive group lookup into the reverse-proxy plugin.

MichaelSp · 2019-09-02T13:43:49Z

Might be a good idea to check if x-forwarded-proto is set. What do you think?

src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java

src/main/java/hudson/plugins/active_directory/HttpHeaderFilter.java

jvz · 2019-09-03T19:06:04Z

This PR is looking decent. I think you might be better off adding the new constructor parameter in the @DataBoundConstructor as a setter method with @DataBoundSetter to avoid having to break backward compatibility with all the constructors you changed. Otherwise, make sure to not delete any old constructors and instead mark them @Deprecated.

src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java

...es/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/help-userFromHTTPHeader.html

.../resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/configAdvanced.jelly

MichaelSp · 2019-09-23T08:53:04Z

don't merge, yet. We've found a bug in the token based authentication.

MichaelSp · 2019-09-23T13:19:42Z

false alert. I just stumbled across this one: https://jenkins.io/security/advisory/2019-07-17/#SECURITY-626 (new crumbIssuer behaviour)

jvz

This generally looks good. I did not test this myself as I do not use AD.

src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java

jvz · 2020-07-13T14:27:37Z

@Wadeck @jtnord thoughts on this?

jtnord

@Wadeck @jtnord thoughts on this?

personally I think this is the incorrect place for this. The upstream reverse proxy that is authenticating should be made to do the recursive lookup, or make the reverse proxy auth plugin support it even though it may not be as simple.

AD can already do this with a matching OID (matching rule in chain) and thus could just be a question of the correct configuration.

on the code itself, left a couple of comments

src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java

src/main/java/hudson/plugins/active_directory/HttpHeaderFilter.java

jtnord · 2020-07-13T15:48:54Z

src/main/java/hudson/plugins/active_directory/HttpHeaderFilter.java

+    }
+
+    private String getUserHeader() {
+        return activeDirectorySecurityRealm.userFromHttpHeader;


if this is the emtpy string how does HttpServletRequest request perform?

from https://docs.oracle.com/javaee/6/api/javax/servlet/http/HttpServletRequest.html#getHeader(java.lang.String)

If the request did not include a header of the specified name, this method returns null.

This case is covered in line 95

I am not talking about the header not being present - I am talking about userFromHttpHeader = ""
thus what does HttpServletRequest.getHeader("") return?

it is certainly undefined at the javadoc level and would (from memory) be in violation of the HTTP spec - so could an implementation potentially throw an arbitrary RuntimeException here?

The running implementation of HttpServletRequest.getHeader is interchangeable.
I've checked it in jetty-9.4.30.v20200611:

https://github.com/eclipse/jetty.project/blob/jetty-9.4.30.v20200611/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java#L852

https://github.com/eclipse/jetty.project/blob/jetty-9.4.30.v20200611/jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java#L270-L279

I don't see the potential for a RuntimeException. Either there is a HTTP-Header that matches "" than it is returned or there is no match. That should yield null. Both cases are handled.

MichaelSp · 2020-07-13T16:08:40Z

thanks for your comment @jtnord
I'm not quite sure how to go on from here. Since the mentioned ticket in reverse proxy auth is open since 5 years and 3 days and the last commit happened 3 years ago, it doesn't seem very plausible to be addressed there.

If it's a matter of configuration, would you please nudge me in the right direction?

Did I mention? We're running this patch since almost 1 year live in production on ~120 Jenkins.

jtnord · 2020-07-13T16:48:38Z

I'm not quite sure how to go on from here. Since the mentioned ticket in reverse proxy auth is open since 5 years and 3 days

the ticket could have been open that long because no one was interested enough to implement it there.

and the last commit happened 3 years ago, it doesn't seem very plausible to be addressed there.

stable code :)
pinging @oleg-nenashev as he is the mainainer there (IIUC)

If it's a matter of configuration, would you please nudge me in the right direction?

to retrieve all groups a user is a member of (member:1.2.840.113556.1.4.1941:=cn=myuser,cn=users,DC=foo)

I don;t have my test setup anymore, but usign that you special OID you can the AD server to do the legwork. (if bob is a member of cheese and cheese is a meber of food then the query will return both cheese and food when using the OID

(in other words - don't add recursive code to the RP plugin (was probably a big mistake of mine to do that at all here), but ask it to get all the results from AD.

oleg-nenashev · 2020-07-13T16:57:08Z

I am not a maintainer here, sorry.

jvz · 2020-07-13T17:02:50Z

@oleg-nenashev he was speaking about https://github.com/jenkinsci/reverse-proxy-auth-plugin which you are a maintainer of

jtnord

Code looks ok - but I still think this should be done in the reverse-proxy plugin.

did you get anywhere with the filters?
https://github.com/jenkinsci/reverse-proxy-auth-plugin/blob/master/src/main/java/org/jenkinsci/plugins/reverse_proxy_auth/ReverseProxySecurityRealm.java#L214-L218

I think something like (member:1.2.840.113556.1.4.1941:={0}) may work - (I'm not sure how the RP plugin works and what filters you would want to use in the various places).

Signed-off-by: Michael Sprauer <[email protected]>

* import settings * Jenkins.getAuthentication * DataBoundSetter and don't mess with the constructors Signed-off-by: Michael Sprauer <[email protected]>

Signed-off-by: Michael Sprauer <[email protected]>

MichaelSp force-pushed the master branch from a72f279 to 7e01e2d Compare September 3, 2019 10:40

jvz reviewed Sep 3, 2019

View reviewed changes

src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java Outdated Show resolved Hide resolved

jvz reviewed Sep 3, 2019

View reviewed changes

src/main/java/hudson/plugins/active_directory/HttpHeaderFilter.java Outdated Show resolved Hide resolved

jvz reviewed Sep 3, 2019

View reviewed changes

src/main/java/hudson/plugins/active_directory/HttpHeaderFilter.java Outdated Show resolved Hide resolved

MichaelSp force-pushed the master branch from f589449 to dd16df5 Compare September 4, 2019 12:41

jvz reviewed Sep 4, 2019

View reviewed changes

src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java Outdated Show resolved Hide resolved

jvz reviewed Sep 4, 2019

View reviewed changes

...es/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/help-userFromHTTPHeader.html Outdated Show resolved Hide resolved

jvz reviewed Sep 4, 2019

View reviewed changes

.../resources/hudson/plugins/active_directory/ActiveDirectorySecurityRealm/configAdvanced.jelly Outdated Show resolved Hide resolved

MichaelSp requested a review from jvz October 17, 2019 20:04

jvz approved these changes Oct 22, 2019

View reviewed changes

src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java Outdated Show resolved Hide resolved

MichaelSp requested a review from jvz July 11, 2020 09:08

jtnord reviewed Jul 13, 2020

View reviewed changes

jtnord reviewed Jul 14, 2020

View reviewed changes

MichaelSp force-pushed the master branch from 86fbab3 to 1801836 Compare January 8, 2021 11:15

MichaelSp added 7 commits January 8, 2021 12:44

support for reverse proxy HTTP-Header based authentication

acb3d8c

Signed-off-by: Michael Sprauer <[email protected]>

Support for "Authentication: basic" header

f2419cc

Signed-off-by: Michael Sprauer <[email protected]>

only set authentication if not yet authenticated

e1d5c8f

Signed-off-by: Michael Sprauer <[email protected]>

incorporate comments from review

9c80756

* import settings * Jenkins.getAuthentication * DataBoundSetter and don't mess with the constructors Signed-off-by: Michael Sprauer <[email protected]>

arghs the constructor trap

4d7f6ab

Signed-off-by: Michael Sprauer <[email protected]>

support regular expression to extract username from http-header field

7c39ec3

Signed-off-by: Michael Sprauer <[email protected]>

changes according to comments

39edf42

Signed-off-by: Michael Sprauer <[email protected]>

MichaelSp added 5 commits January 8, 2021 12:45

do not replace Anon-auth

4d80d21

Signed-off-by: Michael Sprauer <[email protected]>

make usernameExtractionExpression a Pattern

0f86f00

Signed-off-by: Michael Sprauer <[email protected]>

fix tests

9282e28

Signed-off-by: Michael Sprauer <[email protected]>

avoid funky languages :)

ca3003c

Signed-off-by: Michael Sprauer <[email protected]>

restore functionality after rebasing on master

185e9e8

Signed-off-by: Michael Sprauer <[email protected]>

MichaelSp force-pushed the master branch from 1801836 to 185e9e8 Compare January 8, 2021 21:39

yaroslavafenkin pushed a commit to yaroslavafenkin/active-directory-plugin that referenced this pull request Nov 1, 2024

Bump webpack from 5.87.0 to 5.88.2 (jenkinsci#98)

62061e1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for reverse proxy HTTP-Header based authentication #98

support for reverse proxy HTTP-Header based authentication #98

MichaelSp commented Sep 2, 2019

MichaelSp commented Sep 2, 2019

jvz commented Sep 3, 2019

MichaelSp commented Sep 23, 2019

MichaelSp commented Sep 23, 2019

jvz left a comment

jvz commented Jul 13, 2020

jtnord left a comment •

edited

Loading

jtnord Jul 13, 2020

MichaelSp Jul 13, 2020

jtnord Jul 13, 2020 •

edited

Loading

MichaelSp Jul 14, 2020

MichaelSp commented Jul 13, 2020

jtnord commented Jul 13, 2020 •

edited

Loading

oleg-nenashev commented Jul 13, 2020

jvz commented Jul 13, 2020

jtnord left a comment

support for reverse proxy HTTP-Header based authentication #98

Are you sure you want to change the base?

support for reverse proxy HTTP-Header based authentication #98

Conversation

MichaelSp commented Sep 2, 2019

MichaelSp commented Sep 2, 2019

jvz commented Sep 3, 2019

MichaelSp commented Sep 23, 2019

MichaelSp commented Sep 23, 2019

jvz left a comment

Choose a reason for hiding this comment

jvz commented Jul 13, 2020

jtnord left a comment • edited Loading

Choose a reason for hiding this comment

jtnord Jul 13, 2020

Choose a reason for hiding this comment

MichaelSp Jul 13, 2020

Choose a reason for hiding this comment

jtnord Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

MichaelSp Jul 14, 2020

Choose a reason for hiding this comment

MichaelSp commented Jul 13, 2020

jtnord commented Jul 13, 2020 • edited Loading

oleg-nenashev commented Jul 13, 2020

jvz commented Jul 13, 2020

jtnord left a comment

Choose a reason for hiding this comment

jtnord left a comment •

edited

Loading

jtnord Jul 13, 2020 •

edited

Loading

jtnord commented Jul 13, 2020 •

edited

Loading