Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ELY-2023] Elytron ClientCertAuthenticationMechanism does not work when using a web proxy #1442

Merged
merged 1 commit into from Nov 25, 2020

Conversation

rmartinc
Copy link
Contributor

@rmartinc rmartinc commented Sep 16, 2020

Issue: https://issues.redhat.com/browse/ELY-2023

When the certificate mechanism is used behind a web proxy that injects the certificate and SSL info using headers (no cache available), the mechanism uses a AuthorizeCallback that fails (because it creates a NamePrincipal which is different to the original X500Principal). I decided to create a new callback for authorization but using a principal. Let me know if you see something better.

I'm sending the PR against EAP 1.10.x, if you need it against 1.x or other branch please just tell me to do that.

I'm preparing a test PR for wildfly that checks a certificate login using key-store realm.

Wildfly Test PR: wildfly/wildfly#13573

/**
* Indicates if a cached identity was successfully authorized.
*
* @return true if the cached identity was successfully authorized. Otherwise, false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small comment, I think the two references to "cached" above and the reference to "caches" below can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, leftovers from where I copied the class, sorry and done!

Principal principal = authorizeCallback.getPrincipal();
// always re-set the principal to ensure it hasn't changed.
setAuthenticationPrincipal(principal);
boolean authorized = authorize(principal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the authentication principal has already been set and since the same principal is being used for the authorization check, I think just authorize() could be used here instead.

Copy link
Contributor Author

@rmartinc rmartinc Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! Done!

AuthorizeCallback plainCallback = new AuthorizeCallback(name, name);
authorizedFunction = plainCallback::isAuthorized;
authorizeCallBack = plainCallback;
PrincipalAuthorizeCallback principalCallback = new PrincipalAuthorizeCallback(evidence.getDefaultPrincipal());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use evidence.getDecodedPrincipal() instead (getDecodedPrincipal() also takes into account any evidence decoders that have been configured).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@rmartinc
Copy link
Contributor Author

rmartinc commented Oct 2, 2020

Changes made, and I tested with the wildfly test and it keeps passing it smoothly.

}

/**
* Authorizes and caches the given <code>securityIdentity</code>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last change, this should be reworded since there isn't a securityIdentity being passed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argh! I have changed to use always the word principal.

Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @rmartinc.

@fjuma fjuma added the +1 FJ label Oct 5, 2020
@gaol
Copy link
Contributor

gaol commented Oct 26, 2020

FYI: The cloned JBEAP issue: https://issues.redhat.com/browse/JBEAP-20194 has 3 acks now if that is needed :)

@gaol
Copy link
Contributor

gaol commented Oct 26, 2020

@fjuma Hi Farah, do you think we need a PR to 1.x branch as well for WildFly master ? Thanks !

@fjuma
Copy link
Contributor

fjuma commented Oct 26, 2020

@gaol Thanks for letting us know that the JBEAP issue has all acks now. That's all that's needed. We'll handle forwarding porting these changes to the 1.x branch and the master branch as part of our release process. Thanks!

@darranl darranl added the +1 DAL label Nov 24, 2020
@darranl darranl merged commit 972e39e into wildfly-security:1.10.x Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants