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 GLEAM_CACERTS_PATH env variable #3939

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

winstxnhdw
Copy link

@winstxnhdw winstxnhdw commented Dec 2, 2024

Revival of #3459

Tagging @lpil as he was the previous reviewer.

Closes #3246

@winstxnhdw winstxnhdw force-pushed the feat/cacerts-env-var branch from 33063f4 to 56b73d5 Compare December 2, 2024 15:58
@winstxnhdw winstxnhdw marked this pull request as ready for review December 2, 2024 16:15
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!

It looks like these would be used for all HTTP requests rather than just for the Hex API. Is that what we want or would that be too restrictive if we wanted to make requests to other domains too?

How might we test this change?

Comment on lines +57 to +63
fn get_certificate() -> Result<Certificate, Box<dyn std::error::Error>> {
let certificate_path = std::env::var("GLEAM_CACERTS_PATH")?;
let certificate_bytes = std::fs::read(&certificate_path)?;
let certificate = Certificate::from_pem(&certificate_bytes)?;

Ok(certificate)
}
Copy link
Member

Choose a reason for hiding this comment

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

In the event that the certs paths has been set we want to error if the certs cannot be read or parsed, rather than silently discarded as they are here.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think this is usual behaviour.. It's quite common for runtimes like Python and Node to ignore missing certs as it is common behaviour to add the environment variable to your shell and not populate the certificate path until necessary.

@lpil lpil marked this pull request as draft December 2, 2024 17:20
@winstxnhdw
Copy link
Author

winstxnhdw commented Dec 2, 2024

It looks like these would be used for all HTTP requests rather than just for the Hex API. Is that what we want or would that be too restrictive if we wanted to make requests to other domains too?

Again, it seems standard to have the certificates apply to all HTTP requests. In fact, a partial application would be surprising and frustrating behaviour.

@winstxnhdw
Copy link
Author

Hey, any updates?

@lpil
Copy link
Member

lpil commented Dec 4, 2024

I just tested this and node does not silently discard the error, it emits an ugly looking warning.

Make it return an error if an error occurs, thank you.

@winstxnhdw
Copy link
Author

I just tested this and node does not silently discard the error, it emits an ugly looking warning.

Make it return an error if an error occurs, thank you.

I am not sure what you mean. I am returning an error. Can you describe how you reproduced that warning? I am not getting an issue on my end.

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.

Add support to custom CA certificate stores
2 participants