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

fix: gen-pkcs12-keystore adds ca.crt input option if it exists (#684) #685

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: gen-pkcs12-keystore adds ca.crt input option if it exists (#684) #685

wants to merge 6 commits into from

Conversation

smoldenhauer-ish
Copy link
Contributor

No description provided.

@HoustonPutman
Copy link
Contributor

HoustonPutman commented Mar 26, 2024 •
edited
Loading

Thanks for the fix here @smoldenhauer-ish ! Is there a way we can add an integration test, which we already have that use cert-manager, to test this? They currently live in tests/e2e/solrcloud_tls_test.go.

@HoustonPutman HoustonPutman linked an issue Mar 26, 2024 that may be closed by this pull request
gen-pkcs12-keystore init container fails if the tls secret contains no ca.crt #684
Open
@smoldenhauer-ish
Copy link
Contributor Author

I'll need to have a look into solrcloud_tls_test.go and see if I am able to understand it and can add a test in the next days.

HoustonPutman
HoustonPutman reviewed Apr 3, 2024
View reviewed changes
controllers/util/solr_tls_util.go Outdated Show resolved Hide resolved
controllers/solrcloud_controller_tls_test.go Outdated Show resolved Hide resolved
smoldenhauer-ish and others added 2 commits April 4, 2024 08:17
@smoldenhauer-ish @HoustonPutman
fix: gen-pkcs12-keystore adds ca.crt input option if it exists (apach…
3701fad 
…e#684)


* Apply suggestions from code review

Co-authored-by: Houston Putman <houstonputman@gmail.com>
@smoldenhauer-ish
fix: gen-pkcs12-keystore adds ca.crt input option if it exists (apach…
5dcc451 
…e#684)

 * add e2e test generating keystore with init container
@smoldenhauer-ish
Copy link
Contributor Author

smoldenhauer-ish commented Apr 4, 2024 •
edited
Loading

@HoustonPutman applied your suggestions and added a test that will omit the initial creation of the keystore by the cert-manager and invokes the gen-pkcs12-keystore init container. The shell command executes but the openssl fails with the error: "No certificate matches private key"
The test fails even, if I rollback my changes to the command. So it might has something to do with the cert-manager generated certificate and private key.

@smoldenhauer-ish smoldenhauer-ish marked this pull request as draft April 4, 2024 07:01
@smoldenhauer-ish smoldenhauer-ish marked this pull request as ready for review April 5, 2024 11:40
@smoldenhauer-ish
Copy link
Contributor Author

I changed the double "-in" option into "-certfile ca.crt" to add the additional certificate(s) from an ca.crt entry into the created keystore.p12
The double "-in" from the original code is not evaluated by openssl - currently with openssl 1.1.1 the last -in is used.
So it fails, if that is the ca.crt, because there is "No certificate matches private key" the first -in tls.crt is ignored.

With the -certfile option the additional ca.crt is added - the generated keystore.p12 contains the both certificates just like the cert-manager generated keystore.

@smoldenhauer-ish
Copy link
Contributor Author

Hope this is good for merge, now.
BTW, I'm offline for the next two weeks.

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

@HoustonPutman HoustonPutman HoustonPutman left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gen-pkcs12-keystore init container fails if the tls secret contains no ca.crt
2 participants
@smoldenhauer-ish @HoustonPutman