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

MBEDTLSJL_CERT_PEM_* env vars to a more flexible scheme to cert.pem #261

Merged
merged 2 commits into from Dec 12, 2022

Conversation

csantosb
Copy link
Contributor

Not all distributions package cert.pem file along with julia, as it is the case of Archlinux and Guix. The certificates file is to be located somewhere else.

Using two env variables (optional), one may declare where this package may find the cert.pem, whether in a directory, whether in a given absolute location.

@codecov
Copy link

codecov bot commented Dec 10, 2022

Codecov Report

Merging #261 (3468cd2) into master (e224150) will decrease coverage by 0.47%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
- Coverage   73.56%   73.09%   -0.48%     
==========================================
  Files          12       12              
  Lines         715      721       +6     
==========================================
+ Hits          526      527       +1     
- Misses        189      194       +5     
Impacted Files Coverage Δ
src/ssl.jl 71.23% <37.50%> (-1.15%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/ssl.jl Outdated
fallback = abspath(joinpath(Sys.BINDIR, "..", "share", "julia", "cert.pem"))
if isfile(MozillaCACerts_jll.cacert)
DEFAULT_CERT[] = read(MozillaCACerts_jll.cacert, String)
if "MBEDTLSJL_CERT_PEM_FILE" in keys(ENV)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if "MBEDTLSJL_CERT_PEM_FILE" in keys(ENV)
if haskey(ENV, "MBEDTLSJL_CERT_PEM_FILE")

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, did you mean for the if check to be JULIA_CERT_PEM_DIR here?

@quinnj
Copy link
Member

quinnj commented Dec 12, 2022

I'm not sure I quite understand; in the PR, you seem to be checking if the new MBEDTLSJL_ envs are set, but then not using them?

”Haskey” is used instead of ”in” too.
@csantosb
Copy link
Contributor Author

Sorry for the awful PR, I completely missed it ! I fixed it for good, taking into account all your remarks.

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

Great, this makes much more sense. Thanks.

@quinnj quinnj merged commit da061d0 into JuliaLang:master Dec 12, 2022
@DilumAluthge
Copy link
Member

Not all distributions package cert.pem file along with julia, as it is the case of Archlinux and Guix. The certificates file is to be located somewhere else.

This is only the case if you are not using the official Julia binaries. The official binaries will always include the cert.pem file with Julia itself.

Personally, I don't think that we should be adding features like this to Julia packages just to work around people using broken distro Julias. I'd rather we revert this change, and then tell users to use the official Julia binaries.

@quinnj
Copy link
Member

quinnj commented Dec 13, 2022

I think there's still a valid use-case for this; as we recently saw in JuliaWeb/HTTP.jl#947, in private networks w/ self-signed certificates, it's convenient to specify env variables in order to customize which certs get used in the network stack.

@DilumAluthge
Copy link
Member

Sure, but that's a different use case than was brought up in the OP.

Anyway, it would be good to at least standardize the names of these environment variables to match the names used by NetworkOptions (https://github.com/JuliaLang/NetworkOptions.jl).

@khinsen
Copy link

khinsen commented Dec 17, 2022

@DilumAluthge Your comment is rather disrespectful towards packagers. System-level distributions exist for a good reason. Different distributions exist because people's needs differ, in many ways. Packagers often have to modify software to make them fit their distribution's policies, which exist for good reasons. The official Julia binaries don't work with all these distributions, and then there are people who use specific distributions because they don't want to download and run binaries, for reasons ranging from security to reproducibility concerns.

The Julia community can of course decide that it doesn't want to accommodate specific distributions' needs because they would cause additional complexity in Julia. But please don't call distributions or their packages "broken" just because they cater for needs that the official Julia binaries do not satisfy.

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.

None yet

4 participants