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

Add mTLS settings to ApiClient #1415

Merged
merged 23 commits into from
Jan 26, 2024
Merged

Add mTLS settings to ApiClient #1415

merged 23 commits into from
Jan 26, 2024

Conversation

burmanm
Copy link
Collaborator

@burmanm burmanm commented Sep 19, 2023

This can't hot reload yet existing connections, as that requires the connection parameters modifications to be merged first (to use caching). And as such, no such code makes sense before that's done as it would be rewritten.

Fixes #1449

@Miles-Garnsey Miles-Garnsey force-pushed the integration/http-managementproxy branch from d72a78c to e2d1998 Compare October 3, 2023 00:29
@burmanm burmanm changed the base branch from integration/http-managementproxy to master November 23, 2023 17:02
@burmanm
Copy link
Collaborator Author

burmanm commented Nov 23, 2023

Changed base and rebased.

Copy link
Contributor

@Miles-Garnsey Miles-Garnsey left a comment

Choose a reason for hiding this comment

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

Overall I think this looks good. Needs tests though, can we add some?

@Miles-Garnsey
Copy link
Contributor

For some reason the tests aren't running here by the way, hopefully just a GHA glitch and once you add a test for the new functionality they'll run.

@adejanovski
Copy link
Contributor

For some reason the tests aren't running here by the way, hopefully just a GHA glitch and once you add a test for the new functionality they'll run.

Tests didn't run because the target branch wasn't master. I think we need a new commit to be pushed so that CI triggers.

…store

Add two new config options to http, keystore and truststore. They must be mounted as jks keystore files.

If the keystore is set, Reaper tries to build HttpManagementProxy with mTLS enabled.
@adejanovski
Copy link
Contributor

@burmanm, as discussed here's the path forward with this PR:

Modify the http-api integration test suite to use TLS all the time. That'll require adding the encryption related files in tree and modifying these lines to start the mgmt api server with TLS enabled.
Then modify the Reaper configuration for the http api reaper config to set up TLS in Reaper for these tests.
We'll create a subsequent ticket to add a set of test to the matrix without TLS enabled (out of scope for this ticket then).

cc @Miles-Garnsey

@adejanovski
Copy link
Contributor

@Miles-Garnsey this is ready for another review.
Thanks!

Copy link
Contributor

@Miles-Garnsey Miles-Garnsey left a comment

Choose a reason for hiding this comment

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

I've taken a cursory look, this is quite big.

Looks like there's a test failing. I've re-run, let's see if it is just a flake.

src/server/checkstyle.xml Show resolved Hide resolved
@@ -0,0 +1,105 @@
# Copyright 2017-2017 Spotify AB
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Looks like cassandra-reaper-http-at.yaml is no longer used anywhere but still exists. Should we remove it?

@@ -0,0 +1,41 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: looks like we commit in encryption materials above, but generate them here. Any reason for having both approaches?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to regenerate the provided encryption materials.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, why do we want to regenerate them? This is probably fine, just making sure I understand what you're doing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hopefully we wouldn't (or we could modify it later to regenerate on every CI run), but I like to have generators of stuff we use in the repository, in case someone would for example run into an issue on their refactoring - and wonder why something doesn't work. Having the script gives a clear indication how the certs were generated and be easier to debug - instead of having bunch of encrypted things that were done "somehow". In the same way, if this works, but integration from somewhere else doesn't - this could give a clue.

src/server/src/test/resources/openssl.cnf Show resolved Hide resolved
@Miles-Garnsey
Copy link
Contributor

We still have a failing test on this one.

@adejanovski adejanovski merged commit c1491e8 into master Jan 26, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make TLS connection available to management-api
3 participants