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 support for login via certificate #76

Merged
merged 2 commits into from
Mar 1, 2024

Conversation

avandecreme
Copy link

The first commit is cherry picked from #62

The second one is specific to this PR and allow one to use certificate to login to vault.

@Haennetz
Copy link
Collaborator

Haennetz commented Feb 23, 2024

Hey thanks for your pull request. It would be nice if you can add a test for the certificate login. I merged #62 can you rebase your branch then the checks should be able to pass.

Copy link

gitguardian bot commented Feb 27, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@avandecreme
Copy link
Author

I added two commits.
The first one is implementing the tests. The second one is just to show how having jmgilman/dockertest-server#15 merged could clean up this PR.

I committed the keys and certificates as well as the method to generate them. I see that gitguardian is not happy about it even though those are just test certificates.

I guess we could generate new certificates on every test run, provided we can run openssl in the runner. What do you think?

@Haennetz
Copy link
Collaborator

Yes I think generating a new certificate each time would be the best solution for that.
We can use rcgen as a dev dependency and create the certificate with it.

@avandecreme avandecreme force-pushed the certificate_login branch 2 times, most recently from dfcf813 to 8d8e1d2 Compare February 28, 2024 10:58
@avandecreme
Copy link
Author

Yes I think generating a new certificate each time would be the best solution for that. We can use rcgen as a dev dependency and create the certificate with it.

Done

@Haennetz
Copy link
Collaborator

Hey tanks for implementing this. The test is failing at the moment because the nativ-tls feature didn't support pam only. It requires pkcs8 or pkcs12.

@avandecreme
Copy link
Author

I added a commit to disable the test with native-tls since vaultrs does not support client certificates with it anyway. See

error!("Client certificates not implemented for native-tls");

Copy link
Collaborator

@Haennetz Haennetz left a comment

Choose a reason for hiding this comment

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

With the suggestions we should get rid of the warnings.

tests/cert.rs Outdated Show resolved Hide resolved
tests/cert.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@avandecreme
Copy link
Author

avandecreme commented Mar 1, 2024

Suggestions applied

@Haennetz
Copy link
Collaborator

Haennetz commented Mar 1, 2024

Thanks 👍

@Haennetz Haennetz merged commit 4068c18 into jmgilman:master Mar 1, 2024
8 checks passed
@avandecreme avandecreme deleted the certificate_login branch March 1, 2024 16:50
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.

2 participants