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

Moved client cert checking into Authenticator #273

Merged
merged 2 commits into from Jul 6, 2016

Conversation

Projects
None yet
2 participants
@chirino
Contributor

chirino commented Jul 5, 2016

Also support enabling client cert and other auth modes at the same time.

chirino added some commits Jul 5, 2016

Refactor JolokiaHttpsHandler so that it's functionality of checking f…
…or client certs is implemented as an Authenticator instead.

Partially implements #223, but it retains backward compatibility with the current configuration.
Adding an AnyAuthenticator and AllAuthenticator. The AnyAuthenticator…
… is used when you enable basic auth and client cert based auth at the same time.
for (Rdn rdn : principal.getRdns()) {
if (!certPrincipalRdns.contains(rdn)) {
throw new SecurityException("Principal " + certPrincipal + " not allowed");
}

This comment has been minimized.

@rhuss

rhuss Jul 6, 2016

Owner

I will take over the fix from the other PR here.

* @author roland
* @since 26.05.14
*/
public class AllAuthenticator extends Authenticator {

This comment has been minimized.

@rhuss

rhuss Jul 6, 2016

Owner

Just wondering whether an AllAuthenticator makes much sense. Also, it is not used currently, so maybe for KISS reasons we simply omit it now and can easily reintroduce it we allow for a more flexible configuration where an AllAuthenticator could be selected.

@rhuss

This comment has been minimized.

Owner

rhuss commented Jul 6, 2016

LGTM, thanks ;)

I will update the documentation accordingly. A new Jolokia release (1.3.4) is expected at the end of this or beginning of next week.

@rhuss rhuss merged commit 9670a6d into rhuss:master Jul 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rhuss added a commit that referenced this pull request Jul 6, 2016

@rhuss

This comment has been minimized.

Owner

rhuss commented Jul 6, 2016

There will be still a problem for our use case, since the client certificate will be validated still that is signed by a CA. This happens before the authenticators are called, when setting up the HTTPS subsystem.

So I'm afraid that its not possible to have SSL with allowing both, client cert authentication and basic authentication without a prior client cert verification.

The authenticator is used only for some extra checks, like whether the presented certificate is indeed a client certificate (and not a server certificate), and whether the enclosed principal matches a configured value.

The check, whether a client cert is presented and whether it is signed by a given CA (stored in the keystore) happens before, internally in the HttpsServer which we cant influence much except for configuring it.

@chirino

This comment has been minimized.

Contributor

chirino commented Jul 6, 2016

Yeah I think that limitation is ok. In the case a client does not have cert trusted by the CA, he should not do SSL with the cert at all.

@rhuss

This comment has been minimized.

Owner

rhuss commented Jul 6, 2016

But how can you provide a Pod specific client cert signed by the OpenShift CA when running from a Pod ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment