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

feat: add mTLS #39

Merged
merged 4 commits into from
Feb 7, 2024
Merged

feat: add mTLS #39

merged 4 commits into from
Feb 7, 2024

Conversation

M0NsTeRRR
Copy link
Contributor

@M0NsTeRRR M0NsTeRRR commented Jan 28, 2024

This PR adds the possibility to connect to a control agent with mTLS enabled (cert-required). I've also added a missing dependency.
I haven't included the option to pass a custom certificate authority since you can use the REQUESTS_CA_BUNDLE environment variable for that purpose. However, I can add it if you think it's not sufficient.

PS: I've pinned the requests dependency using ~= to prevent installation issues in case of a major release. What are your thoughts on using this approach for all dependencies ?

Signed-off-by: Ludovic Ortega <[email protected]>
Signed-off-by: Ludovic Ortega <[email protected]>
@mweinelt
Copy link
Owner

mweinelt commented Feb 7, 2024

Cool!

I haven't included the option to pass a custom certificate authority since you can use the REQUESTS_CA_BUNDLE environment variable for that purpose. However, I can add it if you think it's not sufficient.

I think we can do this step by step. If someone requests it, it won't be a hassle to add it. But also some short explanation in README.md might help.

PS: I've pinned the requests dependency using ~= to prevent installation issues in case of a major release. What are your thoughts on using this approach for all dependencies ?

I think using a semantic pin where possible and keep the dependencies updated is a good mix to satisfy most consumers.

@mweinelt mweinelt linked an issue Feb 7, 2024 that may be closed by this pull request
@M0NsTeRRR
Copy link
Contributor Author

I've added the 2 new flags in the documentation with some lines about https validation.
I've updated dependencies to follow semver.

@mweinelt
Copy link
Owner

mweinelt commented Feb 7, 2024

You can either squash the "fix: black" commit into the "feat: add mTLS" commit, or I can squash the whole PR. WDYT?

@M0NsTeRRR
Copy link
Contributor Author

M0NsTeRRR commented Feb 7, 2024

I can but on github you can "Squash and merge" to only have one commit and you can override the commit message here to fit repository best practice.

@mweinelt mweinelt merged commit 961207f into mweinelt:develop Feb 7, 2024
2 checks passed
@mweinelt
Copy link
Owner

mweinelt commented Feb 7, 2024

Thank you!

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.

Update dependencies in pyproject.toml
2 participants