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.

Sign up for GitHub

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

Jump to bottom

CORE-3168 support p12 #21313

Merged
merged 6 commits into from
Sep 9, 2024
Merged

CORE-3168 support p12 #21313

merged 6 commits into from
Sep 9, 2024

Conversation

michael-redpanda
Copy link
Contributor

Adds support for using PKCS#12 files in our TLS node config.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

Features

  • Added support for using PKCS#12 files for TLS services

@michael-redpanda michael-redpanda requested a review from a team July 9, 2024 20:28
@michael-redpanda michael-redpanda self-assigned this Jul 9, 2024
@michael-redpanda michael-redpanda requested review from aanthony-rp, BenPope and oleiman and removed request for a team July 9, 2024 20:28
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Jul 9, 2024 •
edited
Loading

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51285#01909987-db34-4048-ad16-d61e783972e3

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51285#01909988-4371-4f37-b903-68ef89793006

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/51285#01909988-4374-4260-96b8-e4a8fa10be48

oleiman
oleiman reviewed Jul 10, 2024
View reviewed changes
Copy link
Member

@oleiman oleiman left a comment

Choose a reason for hiding this comment

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

lgtm. several comments, all nitpicks

src/v/config/tests/tls_config_convert_test.cc Outdated Show resolved Hide resolved
src/v/config/tls_config.cc Outdated Show resolved Hide resolved
src/v/config/tls_config.cc Outdated Show resolved Hide resolved
src/v/config/tls_config.h Outdated Show resolved Hide resolved
tests/rptest/services/tls.py Outdated Show resolved Hide resolved
tests/rptest/services/tls.py Outdated Show resolved Hide resolved
tests/rptest/tests/pkcs12_test.py Outdated Show resolved Hide resolved
aanthony-rp
aanthony-rp previously approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@aanthony-rp aanthony-rp left a comment

Choose a reason for hiding this comment

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

This adds support for P12 password files, and also seems to extend the implementation to accept multiple key/cert file pairs instead of just a single pair. The test coverage looks good and I don't see any controversial points.

@michael-redpanda
Copy link
Contributor Author

Force push 4818ef6:

  • Updates from PR comments

oleiman
oleiman previously approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@oleiman oleiman left a comment

Choose a reason for hiding this comment

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

👍

src/v/config/tls_config.cc Outdated Show resolved Hide resolved
aanthony-rp
aanthony-rp previously approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@aanthony-rp aanthony-rp left a comment

Choose a reason for hiding this comment

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

I'll trust Oren and Ben to approve the changes they requested earlier.

@michael-redpanda michael-redpanda dismissed stale reviews from aanthony-rp and oleiman via 12ea872 July 12, 2024 15:12
@michael-redpanda
Copy link
Contributor Author

Force push 12ea872:

  • Updates from PR commet

@michael-redpanda
configuration: Add support for P12 files
cb10bc4 
PKCS#12 files (or PFX files) can be used to hold various cryptographic
keys and certificates in a secure way.  These files are encrypted using
a user provided password and allows for secure transmission of keys and
certs from an issuer to an endpoint.  This commit allows for users of
Redpanda to use a P12 file (with a supplied password for decryption)
instead of a plain-text key and cert when setting up TLS configurations.

Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
dt: Cleanup of tls.py
28a09c8 
Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
dt: Added ability to generate P12 files
5151053 
Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
dt: Added saving of PKCS12 file to node
c50a2c5 
Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
dt: Added plumbing for PKCS12 file
18ddeb1 
Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
dt: Added pkcs12 test
90d4bdb 
Added PKCS#12 file smoke test

Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
Copy link
Contributor Author

Force push 90d4bdb:

  • One more nit fix

aanthony-rp
aanthony-rp approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@aanthony-rp aanthony-rp left a comment

Choose a reason for hiding this comment

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

lgtm

@michael-redpanda michael-redpanda requested a review from a team September 5, 2024 14:54
@Deflaimun
Copy link
Contributor

Hi @michael-redpanda what do we need to review for docs? I don't see any user-facing strings.

@mattschumpert
Copy link

There are 2 new configs AIUI.

cc @deniscoady who can help with docs

@michael-redpanda michael-redpanda merged commit 17fb0e6 into redpanda-data:dev Sep 9, 2024
19 checks passed
@michael-redpanda
Copy link
Contributor Author

Hi @michael-redpanda what do we need to review for docs? I don't see any user-facing strings.

@Deflaimun I filed a docs issue (https://redpandadata.atlassian.net/browse/DOC-539) for this new feature, so it's more of an FYI

dotnwat
dotnwat reviewed Sep 9, 2024
View reviewed changes
src/v/config/tls_config.cc
}

return std::nullopt;
}

std::ostream& operator<<(std::ostream& o, const config::p12_container& p) {
fmt::print(o, "{{ p12 file: {}, p12 password: REDACTED }}", p.p12_password);
Copy link
Member

Choose a reason for hiding this comment

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

@michael-redpanda did you intend to print the file, rather than the password?

Copy link
Member

Choose a reason for hiding this comment

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

presumably, yeah, but Mike can confirm. PR because it's a trivial: #23249

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

@oleiman oleiman oleiman approved these changes

@dotnwat dotnwat dotnwat left review comments

@aanthony-rp aanthony-rp aanthony-rp approved these changes

@BenPope BenPope BenPope approved these changes

Assignees

@michael-redpanda michael-redpanda

Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@michael-redpanda @vbotbuildovich @Deflaimun @mattschumpert @dotnwat @BenPope @oleiman @aanthony-rp