-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[ENH][SEC]: CIP-01022024 SSL Verify Client Config #1604
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
…version require it. - Added openssl.cnf in tests
@HammadB, all issues resolved. Ready to go, PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a really good feature to add. I asked a couple minor questions -- let's get this bad boy checked in later today.
## Public Interfaces | ||
|
||
New settings variable `chroma_server_ssl_verify` accepting either a boolean or a path to a certificate file. If the | ||
value is a path to a certificate file, the file will be used to verify the server's certificate. The value is passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the value is a boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean means just ignore (false
) or enforce (true
) SSL cert verification. I'll document that.
|
||
## Public Interfaces | ||
|
||
New settings variable `chroma_server_ssl_verify` accepting either a boolean or a path to a certificate file. If the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we specify that this settings variable only matters for clients? And probably document that fact in a comment next to the settings variable itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can go even further and add this to the official docs. Let me quickly have a look to see if it makes sense to add it somewhere.
@beggers, I think this is ready, but we need to add JS client parity here or in a quick follow-up. Wdyt? |
- Fixed and issue with isomorphic-fetch in the runtime. - Added scripts for creating SSL certs and running Chroma SSL server. - Added SSL tests. Refs: chroma-core#1604
@beggers, JS SSL support was added in a separate stacked PR. |
- Fixed utils to support isomorphic-fetch Response - Added some cleanups Refs: chroma-core#1604
- Fixed and issue with isomorphic-fetch in the runtime. - Added scripts for creating SSL certs and running Chroma SSL server. - Added SSL tests. Refs: chroma-core#1604
- Fixed utils to support isomorphic-fetch Response - Added some cleanups Refs: chroma-core#1604
## Description of changes *Summarize the changes made by this PR.* - New functionality - New CIP to introduce SSL verify flag to support custom PKIs or to accept self-signed certs for testing and experimentation purposes ## Test plan *How are these changes tested?* - [x] Tests pass locally with `pytest` for python ## Documentation Changes CIP document in the PR.
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytest
for pythonDocumentation Changes
CIP document in the PR.