Skip to content

Conversation

@knoppiks
Copy link
Contributor

@knoppiks knoppiks commented Jul 7, 2023

This PR tries to enable the dynamiclistener to hand out not only the first signing certificate it finds in the CA chain.

I stumbled on this discussion while searching for the exact problem in k3s, where @brandond pointed to this repository.

Maybe it would be good to add tests but being not a go expert I struggle to comprehend the tests conducted on the listener.

@brandond
Copy link
Member

brandond commented Jul 7, 2023

This looks good structurally, however I don't think we can change the call signatures of existing exported functions in order to avoid breaking users of this library. We'd probably need to modify this PR to include new functions that return []x509.Certificate, use those to load the cert, instead of simply changing the existing ones.

@knoppiks knoppiks force-pushed the multiple-ca-certs branch from 1bfb2bc to 9c4bda5 Compare July 10, 2023 06:59
@knoppiks knoppiks force-pushed the multiple-ca-certs branch from 9c4bda5 to 0230404 Compare July 10, 2023 07:42
@knoppiks
Copy link
Contributor Author

@brandond Please have a look. I tried my best with the naming, maybe you have better suggestions.

Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

couple nits on error handing and naming, looks good otherwise!

@knoppiks knoppiks force-pushed the multiple-ca-certs branch from 4f561ec to e4ce4d0 Compare July 14, 2023 06:25
knoppiks and others added 2 commits July 14, 2023 08:28
Co-authored-by: Brad Davidson <[email protected]>
Signed-off-by: Jonas Wagner <[email protected]
Co-authored-by: Brad Davidson <[email protected]>
Signed-off-by: Jonas Wagner <[email protected]>
@knoppiks knoppiks force-pushed the multiple-ca-certs branch from e4ce4d0 to 6cc9a67 Compare July 14, 2023 06:29
@brandond brandond requested review from a team July 15, 2023 01:22
@knoppiks
Copy link
Contributor Author

Any news here? I'd really like to tackle the k3s portion of this fix.

@caroline-suse-rancher
Copy link

Hey @knoppiks we're in code freeze at the moment for July releases, but after that we'll be looking for one more reviewer/approver for this PR. I will bring it up with the team after freeze is lifted!

@brandond
Copy link
Member

brandond commented Jul 28, 2023

We can get it approved and merged here, and then hold off on updating anything in K3s until after the freeze is over. This repo is not subject to code freeze.

@dereknola dereknola merged commit e6585da into rancher:master Aug 11, 2023
@knoppiks knoppiks deleted the multiple-ca-certs branch August 15, 2023 12:26
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.

5 participants