Extract out environment verification logic and add optional Spring Security component #8

dcrissman · 2017-12-15T15:58:04Z

No description provided.

derek63

Looks good overall, mostly naming/organizational changes, and a logic fix or two

derek63 · 2017-12-15T18:16:26Z

src/main/java/org/esbtools/auth/util/EnvironmentUtils.java

+        }
+    }
+
+    public String getLDAPAttribute(String certificatePrincipal, String searchAttribute) throws NamingException {


Maybe move this method to LdapRolesProvider, since it doesn't really anything to do with environment.

Environment doesn't know about any of the RolesProviders, I would like to keep it that way. Environment is already acting on DNs, so I don't think it is out of line to have this method here.

derek63 · 2017-12-15T18:17:00Z

src/main/java/org/esbtools/auth/util/EnvironmentUtils.java

+        this.allAccessOu = allAccessOu;
+    }
+
+    public void validateEnvironment(String certificatePrincipal) throws NamingException {


If we change class name to Environment, this can just be called validate(String...

derek63 · 2017-12-15T18:17:20Z

src/main/java/org/esbtools/auth/util/EnvironmentUtils.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class EnvironmentUtils {


Maybe just call this Environment

derek63 · 2017-12-15T18:23:47Z

src/main/java/org/esbtools/auth/util/EnvironmentUtils.java

+    }
+
+    public EnvironmentUtils(String environment) {
+        this(environment, null);


Add Objects.notNull(environment) here, null environment not valid here too

derek63 · 2017-12-15T18:23:49Z

src/main/java/org/esbtools/auth/util/EnvironmentUtils.java

+
+    public EnvironmentUtils(String environment, String allAccessOu) {
+        if (environment == null) {
+            throw new NullPointerException("environment cannot be null");


Maybe just use Objects.notNull(environment) here

derek63 · 2017-12-15T18:36:51Z

src/main/java/org/esbtools/auth/spring/CertEnvironmentVerificationFilter.java

+
+  @Override
+  public void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws IOException, ServletException {
+    if (null != envUtils) {


You also want to check if what is configured is not an empty string (i.e. ""). In the past an empty environment signified you could skip environment validation. I think that the way this is currently written, an empty environment config would still be validated (because it is not null).

We could also choose to interpret null for the environment configuration as "doesn't need to be validated", we just need to be consistent and allow for that in CertLoginLdapLoginModule (currently requires environment to be present in config) and whatever else uses this.

We could instead a method to the Environment class called requiresValidation() that returns false when the value is null or empty string, and true otherwise (I think I like this option the best). We could also check the allAccessOu value in that method too since that is another factor in determining whether or not to do validation logic.

You also want to check if what is configured is not an empty string (i.e. "")

Do we need to handle blank spaces also? eg. " "

We could also choose to interpret null for the environment configuration as "doesn't need to be validated"

That is exactly what CertEnvironmentVerificationFilter does, it just handles it at that layer instead of in Environment. I did it at that level because I didn't have enough context to make changes to the logic flow of CertLdapLoginModule.

If we want this to be default behaviour, then it can definitely be moved to the Environment / requiresValidation.

derek63

A few more minor changes, but looks good overall

derek63 · 2018-01-02T10:55:55Z

src/main/java/org/esbtools/auth/jboss/CertLdapLoginModule.java


+    private static volatile Environment envUtils;


Probably want to rename variable too to remove the Utils from the name

derek63 · 2018-01-02T11:03:19Z

I think we should modify this project to product two separate artifacts:

jboss-cert-ldap-login-module - Jboss login module and common code/libraries

spring-cert-ldap-login-module - Spring Security classes/config and common code/libraries

It feels weird to pull in a library with jboss in the name to do spring security (and vice versa).

FYI, I updated the name of this project this morning to remove "jboss" from the name since we have more than just that now

Spring iterate

dcrissman · 2018-01-02T17:25:44Z

I think we should modify this project to product two separate artifacts:

Change has been made. I plan to bump the major version when I go to release to 2.0.0

dcrissman

Aside from my comments, this is ready for another review.

dcrissman · 2018-01-02T17:37:41Z

jboss-cert-ldap-login-module/pom.xml

+      <artifactId>commons-lang</artifactId>
+      <version>2.6</version>
+      <scope>provided</scope>
+    </dependency>


I hate this block of dependencies. Is there at least a pom we could import to control these versions?

Even if we did an import, it would lock us to a specific version of EAP dependencies, which I am not a fan of. I would like to just delete this block and force consuming applications to deal with specifics if needed.

No idea if there is a pom to import these versions from somewhere (probably not).

Not sure if I understand how "delete this block and force consuming applications to deal with specifics if needed" would actually work since the project would no longer compile and/or generate runtime errors if you did that.

Perhaps I am not fully understanding what you're suggesting

I got a little carried away. Looking again, it is only slf4j that could be removed as it is defined in the parent pom at version 1.7.25. The other dependencies would need to remain for exactly the reason that you stated. Sorry.

As for dealing with specifics, I guess my question is what version of EAP is this targeting? Different versions of EAP are going to provide different versions of these dependencies, which has the potential to cause runtime errors. I was suggesting we remove the provided scope, use the latest and greatest versions if we choose, and let consumers dependencyManage if needed, rather than trying to guess what they might need.

Okay, you bring up a good point about the versions though...I agree the with your suggestion that removing provided and using latest and greatest is probably best bet here. I can't remember why these got marked as provided in the first place, let's make sure to test this in JBoss when it is finished to make sure removing provided doesn't cause runtime issues

Ideally this should work in either version of EAP (which I think it does currently)

I got a little carried away.

Or did I?

commons-lang and guava are defined in the common library at versions 2.6 and 19.0 respectively. So the redefinition in jboss-cert-ldap-login-module is only to enforce a version and provided scope. Which seems ugly to me.

If we're rolling with non-provided versions, then they can/should only exist in common I would think

dcrissman · 2018-01-02T18:46:18Z

jboss-cert-ldap-login-module/src/main/java/org/esbtools/auth/jboss/CertLdapLoginModule.java

            } else {
                // fallback to getting search name from cn in certificate principle (legacy certificates)
-                searchName = getLDAPAttribute(certPrincipal, CN);
+                searchName = environment.getLDAPAttribute(certPrincipal, CN);


No validation happens in this case (as was the way I found it). Should it?

I think we can actually remove this whole if/else since we don't need to support legacy certificates anymore. So just:

String searchName = environment.getLDAPAttribute(certPrincipal, UID);
environment.validate(certPrincipal);

dcrissman · 2018-01-02T18:59:08Z

jboss-cert-ldap-login-module/src/main/java/org/esbtools/auth/jboss/CertLdapLoginModule.java

            } else {
                // fallback to getting search name from cn in certificate principle (legacy certificates)
-                searchName = getLDAPAttribute(certPrincipal, CN);
+                searchName = environment.getLDAPAttribute(certPrincipal, CN);
            }

            Collection<String> groupNames = rolesProvider.getUserRoles(searchName);


searchName could be null here. LdapRolesProvider asserts that it is not null and will throw a NPE if it is. Should we assume that if searchName == null that we should skip the check for the jboss implementation? The spring implementation is a little different, but if a cert cannot be found in the request, we assume that this check should be skipped.

We need to guard against an NPE, we should probably explicitly throw an not authorized exception if a UID can't be found in the principal.

If there is a value defined for environment in config, we should never skip environment validation for any request. If a user doesn't provide a certificate in the request, authentication should fail (always). In fact wouldn't it have already failed before we got to this stage?

If there is a value defined for environment in config, we should never skip environment validation for any request.

This is different than how the spring implementation will work. For that solution, we assume that it is possible for multiple kinds of authentication to happen independently or together. So in that case, we have to assume that if a cert is not provided, that another form of authn could be being used.

My preference would be for both implementations to logically work the same.

But isn't this condition triggered when they present a cert, but it doesn't have a UID? They would never get to this code if they hadn't already authorized with a certificate (happens before authz in login module lifecycle.

I don't know how it works in jboss, maybe. In spring, the check is being added to the filter chain and so will always fire. So maybe that is a difference in the two libraries and just needs to be documented?

dcrissman · 2018-01-02T19:14:24Z

spring-cert-ldap-login-module/pom.xml

+      <groupId>javax.servlet</groupId>
+      <artifactId>servlet-api</artifactId>
+      <version>2.5</version>
+      <scope>provided</scope> <!-- TODO provided? -->


Should this be provided? I left it so containers could provide their own. For example, EAP (for whatever reason) provides it's own version of the apis.

Yes java.servlet should be provided

dcrissman · 2018-01-02T19:14:58Z

spring-cert-ldap-login-module/pom.xml

+      <groupId>javax.ws.rs</groupId>
+      <artifactId>javax.ws.rs-api</artifactId>
+      <version>2.1</version>
+      <scope>provided</scope> <!-- TODO provided? -->


Same question here.

Yes, provided

dcrissman · 2018-01-02T19:15:22Z

org.esbtools.auth.common/pom.xml

+      <artifactId>servlet-api</artifactId>
+      <version>2.5</version>
+      <scope>provided</scope> <!-- TODO provided? -->
+    </dependency>


Should this be provided? I left it so containers could provide their own. For example, EAP (for whatever reason) provides it's own version of the apis.

The javax.servlet, commons-lang, and guava can be marked provided, the unboundid one cannot.

derek63 · 2018-01-03T11:56:09Z

jboss-cert-ldap-login-module/README.md

+	<jsse keystore-password="<password>" keystore-url="file://${jboss.server.config.dir}/keystore.jks" truststore-password="<password>" truststore-url="file://${jboss.server.config.dir}/truststore.jks" client-auth="true"/>
+  </security-domain>
+</subsystem>
+```


Please add some usage information here for the Spring-based client

I will create a README.md in the spring module.

Please add the spring details to this file instead of having them in a separate file, so it is visible on the main page of the repository

derek63 · 2018-01-03T13:16:53Z

jboss-cert-ldap-login-module/pom.xml

+      <artifactId>commons-lang</artifactId>
+      <version>2.6</version>
+      <scope>provided</scope>
+    </dependency>


No idea if there is a pom to import these versions from somewhere (probably not).

Not sure if I understand how "delete this block and force consuming applications to deal with specifics if needed" would actually work since the project would no longer compile and/or generate runtime errors if you did that.

Perhaps I am not fully understanding what you're suggesting

derek63 · 2018-01-03T13:21:37Z

jboss-cert-ldap-login-module/src/main/java/org/esbtools/auth/jboss/CertLdapLoginModule.java

            } else {
                // fallback to getting search name from cn in certificate principle (legacy certificates)
-                searchName = getLDAPAttribute(certPrincipal, CN);
+                searchName = environment.getLDAPAttribute(certPrincipal, CN);


I think we can actually remove this whole if/else since we don't need to support legacy certificates anymore. So just:

String searchName = environment.getLDAPAttribute(certPrincipal, UID);
environment.validate(certPrincipal);

derek63 · 2018-01-03T13:26:54Z

jboss-cert-ldap-login-module/src/main/java/org/esbtools/auth/jboss/CertLdapLoginModule.java

            } else {
                // fallback to getting search name from cn in certificate principle (legacy certificates)
-                searchName = getLDAPAttribute(certPrincipal, CN);
+                searchName = environment.getLDAPAttribute(certPrincipal, CN);
            }

            Collection<String> groupNames = rolesProvider.getUserRoles(searchName);


We need to guard against an NPE, we should probably explicitly throw an not authorized exception if a UID can't be found in the principal.

If there is a value defined for environment in config, we should never skip environment validation for any request. If a user doesn't provide a certificate in the request, authentication should fail (always). In fact wouldn't it have already failed before we got to this stage?

derek63 · 2018-01-03T13:29:34Z

org.esbtools.auth.common/pom.xml

+      <artifactId>servlet-api</artifactId>
+      <version>2.5</version>
+      <scope>provided</scope> <!-- TODO provided? -->
+    </dependency>


The javax.servlet, commons-lang, and guava can be marked provided, the unboundid one cannot.

derek63 · 2018-01-03T13:41:01Z

spring-cert-ldap-login-module/pom.xml

+      <groupId>javax.servlet</groupId>
+      <artifactId>servlet-api</artifactId>
+      <version>2.5</version>
+      <scope>provided</scope> <!-- TODO provided? -->


Yes java.servlet should be provided

derek63 · 2018-01-03T13:41:08Z

spring-cert-ldap-login-module/pom.xml

+      <groupId>javax.ws.rs</groupId>
+      <artifactId>javax.ws.rs-api</artifactId>
+      <version>2.1</version>
+      <scope>provided</scope> <!-- TODO provided? -->


Yes, provided

derek63 · 2018-01-03T14:09:53Z

...s.auth.common/src/main/java/org/esbtools/auth/servlet/CertEnvironmentVerificationFilter.java

+       ((HttpServletResponse) response).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+    }
+
+    response.setContentType("text/html");


Should this maybe be JSON instead of HTML so it is more easily parsable by clients?

I thought about that as well. The reason I used html is to be consistent with undertow, which uses html in all of its filter level responses.

My solution is a little convoluted here, but in the spring module I overwrote this class to throw an exception instead, which is caught by yet another filter. I am working on a hopefully better solution, but it might need to be a follow up to this PR.

I hear you on consistency, I always hated how Undertow (and prior JBoss versions with tomcat) handled this, as it is painful for the clients who are expecting a JSON response

Or I should say that @bvulaj is working on some changes that could simplify the CertEnvironmentVerificationFilter. So maybe this could happen in a follow up PR?

This is not an essential change for this PR, as we have been living with this behavior for over three years now from JBoss, and don't get that many complaints. Good to know @bvulaj is working on more changes to the CertEnvironmentVerificationFilter, let's pick this discussion back up when those are complete.

derek63 · 2018-01-03T14:47:55Z

...s.auth.common/src/main/java/org/esbtools/auth/servlet/CertEnvironmentVerificationFilter.java

+    LOGGER.debug("Attempting Environment Cert verification");
+    X509Certificate certChain[] = (X509Certificate[]) request.getAttribute("javax.servlet.request.X509Certificate");
+
+    if ((null != certChain) && (certChain.length > 0)) {


Doesn't this line ensure that the environment validation check will never happen if the client doesn't present a certificate?

You are correct, that is exactly what it does. The filter itself fires every time, regardless of if a cert is provided or not. But the validation only happens if a cert is present.

I guess I think of CertEnvironmentVerificationFilter as being akin to CertLdapLoginModule. But perhaps it is more nuanced then that?

dcrissman · 2018-01-03T16:24:20Z

Ready for another round of reviews.

derek63

Just a few minor/nitpicky things left and I think this will be ready to merge

derek63 · 2018-01-04T12:31:08Z

jboss-cert-ldap-login-module/README.md

+	<jsse keystore-password="<password>" keystore-url="file://${jboss.server.config.dir}/keystore.jks" truststore-password="<password>" truststore-url="file://${jboss.server.config.dir}/truststore.jks" client-auth="true"/>
+  </security-domain>
+</subsystem>
+```


Please add the spring details to this file instead of having them in a separate file, so it is visible on the main page of the repository

derek63 · 2018-01-04T12:33:56Z

jboss-cert-ldap-login-module/src/main/java/org/esbtools/auth/jboss/CertLdapLoginModule.java

@@ -160,15 +152,13 @@ public void initializeRolesProvider() throws Exception {
            LOGGER.debug("Certificate principal:" + certPrincipal);

            //first try getting search name from uid in certificate principle (new certificates)


You can probably remove this comment now that there is only one way after removing support for legacy certificates

derek63 · 2018-01-04T12:35:33Z

jboss-cert-ldap-login-module/src/main/java/org/esbtools/auth/jboss/CertLdapLoginModule.java

-                searchName = getLDAPAttribute(certPrincipal, CN);
+            String searchName = environment.getLDAPAttribute(certPrincipal, UID);
+            if (StringUtils.isBlank(searchName)) {
+                throw new LoginException("A certificate is required.");


Please update exception message to "A certificate with a UID attribute in the subject name is required."

derek63 · 2018-01-04T12:38:11Z

...s.auth.common/src/main/java/org/esbtools/auth/servlet/CertEnvironmentVerificationFilter.java

+       ((HttpServletResponse) response).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+    }
+
+    response.setContentType("text/html");


This is not an essential change for this PR, as we have been living with this behavior for over three years now from JBoss, and don't get that many complaints. Good to know @bvulaj is working on more changes to the CertEnvironmentVerificationFilter, let's pick this discussion back up when those are complete.

derek63 · 2018-01-04T12:39:15Z

org.esbtools.auth.common/src/test/java/org/esbtools/auth/CachedRolesProviderTest.java

@@ -16,7 +16,7 @@
 You should have received a copy of the GNU General Public License
 along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
-package com.redhat.auth;
+package org.esbtools.auth;


Oops, thanks for fixing this :-)

derek63 · 2018-01-04T12:39:36Z

...th.common/src/test/java/org/esbtools/auth/servlet/CertEnvironmentVerificationFilterTest.java

@@ -0,0 +1,99 @@
+package org.esbtools.auth.servlet;


Hooray tests for this stuff! 👍

derek63 · 2018-01-04T12:42:35Z

pom.xml

-      <groupId>org.apache.directory.server</groupId>
-      <artifactId>apacheds-server-integ</artifactId>
-      <version>1.5.7</version>
+      <version>${slf4j.version}</version>


I think we should either update all version values to use properties or have them all inline. This one has a property and most of the others are inline (I know I am probably the one who did this originally, so sorry for getting nitpick about this just now).

dcrissman · 2018-01-04T14:07:41Z

Requested changes made. Ready for another review.

derek63

Looks good, thanks!

dcrissman added 3 commits December 15, 2017 08:48

fix test packages

e45a463

move over spring classes

5f1593e

refactor out comon environment logic

3f4b62c

derek63 requested changes Dec 15, 2017

View reviewed changes

dcrissman added 5 commits December 15, 2017 14:04

requested changes

9a38566

fix NPE

5b74c79

refactor and add testing for CertEnvironmentVerificationFilter

5f68663

refactors

3e2a90b

clean up

8a6a0e8

derek63 requested changes Jan 2, 2018

View reviewed changes

dcrissman and others added 4 commits January 2, 2018 09:31

rename variable

70afbdb

merge in upstream

c24e963

Merge pull request #1 from dcrissman/spring-iterate

f570478

Spring iterate

extract out multiple modules

f7fd8c9

dcrissman added 3 commits January 2, 2018 12:36

remove files

b73c857

move blank check to common

810a249

unit test fixes

bee312a

dcrissman commented Jan 2, 2018

View reviewed changes

derek63 requested changes Jan 3, 2018

View reviewed changes

derek63 reviewed Jan 3, 2018

View reviewed changes

dcrissman added 2 commits January 3, 2018 10:30

requested changes

8231c25

READMEs

42664c8

add unit test

917fd69

derek63 requested changes Jan 4, 2018

View reviewed changes

dcrissman added 2 commits January 4, 2018 09:03

requested changes:

f146d7b

rename common module

830a10c

derek63 approved these changes Jan 4, 2018

View reviewed changes

naming fixes

6b10ba9

dcrissman merged commit 259302b into esbtools:master Jan 4, 2018

dcrissman deleted the spring branch January 4, 2018 14:43

		@@ -160,15 +152,13 @@ public void initializeRolesProvider() throws Exception {
		LOGGER.debug("Certificate principal:" + certPrincipal);

		//first try getting search name from uid in certificate principle (new certificates)

Extract out environment verification logic and add optional Spring Security component #8

Extract out environment verification logic and add optional Spring Security component #8

Conversation

dcrissman commented Dec 15, 2017

derek63 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derek63 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derek63 commented Jan 2, 2018 • edited Loading

dcrissman commented Jan 2, 2018

dcrissman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcrissman commented Jan 3, 2018

derek63 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcrissman commented Jan 4, 2018

derek63 left a comment

Choose a reason for hiding this comment

derek63 commented Jan 2, 2018 •

edited

Loading