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

SOLR-15423 JWTAuthPlugin support for custom truststore #139

Merged
merged 41 commits into from Jun 10, 2021

Conversation

janhoy
Copy link
Contributor

@janhoy janhoy commented May 21, 2021

Signed-off-by: Jan Høydahl <jan.git@cominvent.com>
…erts to trust when talking to IdPs

Signed-off-by: Jan Høydahl <jan.git@cominvent.com>
Signed-off-by: Jan Høydahl <jan.git@cominvent.com>
@janhoy janhoy requested a review from epugh May 21, 2021 07:36
@janhoy
Copy link
Contributor Author

janhoy commented May 21, 2021

Feedback welcome. I could not think of a good way to actually test that the certificates are used (do we have a mock HTTPS server somewhere?).

@janhoy
Copy link
Contributor Author

janhoy commented May 21, 2021

Committed latest changes. Please resolve outstanding conversations if you're happy. Still would like some integration test to check the actual certificate usage. Think I'll try to spin up a MockIdp listening on some SSL socket...

@janhoy
Copy link
Contributor Author

janhoy commented May 26, 2021

I'm trying to integrate https://github.com/navikt/mock-oauth2-server into the tests (actually open-sourced by a Norwegian government entity). Quite simple, you initialize a MockOAuth2Server and then it listens on a separate port and behaves like an Oauth server. I'm struggling to make it talk https though, so that we can finally assert that the new config options actually works.

If this ends up working, we can also add several other end-to-end integration tests for JWT auth (in separate issues).

@janhoy janhoy marked this pull request as draft May 26, 2021 14:20
@janhoy
Copy link
Contributor Author

janhoy commented May 28, 2021

I just integrated mock-oauth2-server, and finally was able to verify that you can configure JWTAuthPlugin with a PEM file and it is accepted by the mock server. Still have some cleanup to do

  • Create a negative test with a different cert, to see it failing
  • Reconsider use of SslFactory dependency and BouncyCastle to setup SSL. May instead check in a .jks or .p12 file for the tests.
  • Other feedback welcome

@janhoy janhoy marked this pull request as ready for review June 4, 2021 13:18
@janhoy
Copy link
Contributor Author

janhoy commented Jun 4, 2021

I have now addressed my two points

  • Create a negative test with a different cert, to see it failing
  • Remove need for BouncyCastle by importing pcs12 file directly in test

I have also added license and sha files for all new test dependencies and made precommit pass.
I think this is now ready for review.
It's a large PR, but most files are really SHA, LICENSE and NOTICE files.

Copy link
Contributor

@madrob madrob left a comment

Choose a reason for hiding this comment

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

Overall looks good, few minor comments. I did not run tests on this.

solr/solr-ref-guide/src/jwt-authentication-plugin.adoc Outdated Show resolved Hide resolved
solr/solr-ref-guide/src/jwt-authentication-plugin.adoc Outdated Show resolved Hide resolved
public void infoRequestWithoutToken() throws Exception {
get(baseUrl + "/admin/info/system", null);
@Test
public void extractCertificateFromPem() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a test where the pen file or directly supplied string is improperly formatted? I'd like to see what kind of error message users can expect in that case (and maybe we should document that, because I suspect it will be cryptic otherwise)

Copy link
Contributor Author

@janhoy janhoy Jun 7, 2021

Choose a reason for hiding this comment

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

I added two tests to JWTAuthPluginTest, verifying that init() fails. No need for integration test of all these?

If you load a security.json to ZK with bogous trustedCertsFile or trustedCerts (or other config) while Solr is running, the JWT plugin init() will throw and exception and auth will not be enabled/changed. Here are log messages you can expect to see prior to exception:

Reading trustedCerts from file {}

or

Reading trustedCerts PEM from configuration string

In the exception stacktrace itself you'll see either

Wrong type of certificates. Must be DER or PEM format

or

Failed loading certificate(s) from input stream

If you start Solr from scratch with a bogous config, I think the server will fail during CoreContainer init and not start at all. The fact that users need to monitor Solr logs when pushing new security.json is unfortunate. It is easy to make mistakes when automating this with scripts. You may start Solr, then apply security.json which fails, and then you have an unsecured cluster without even knowing :(

@janhoy
Copy link
Contributor Author

janhoy commented Jun 10, 2021

BUILD SUCCESSFUL in 36m 16s
I'll merge this now

@janhoy janhoy merged commit 11ce8b8 into apache:main Jun 10, 2021
@janhoy janhoy deleted the SOLR15423-jwt-truststore branch June 10, 2021 10:31
epugh pushed a commit to epugh/solr that referenced this pull request Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants