search

pantsbuild / setup

Scripts for setting up Pants in your repo
Apache License 2.0
13 stars 30 forks source link

Enforce use of HTTPS with >= TLS 1.2 for curl calls #117

Closed christophermaier closed 2 years ago

christophermaier commented 2 years ago

This is a follow-on to https://github.com/pantsbuild/setup/issues/116, tightening up the security of the curl calls made in the pants script.

I also took the liberty of replacing single-character options with their long option equivalents for readability.

Signed-off-by: Christopher Maier chris@graplsecurity.com

benjyw commented 2 years ago

Thanks for this!

My one concern is the --tlsv1.3 flag. On my macbook it makes curl fails with this error: curl: (4) LibreSSL was built without TLS 1.3 support

So I fear widespread breakage on systems where curl does not support TLS 1.3 yet.

benjyw commented 2 years ago

Maybe my system is unusual in some way? I'd be interested in seeing if this is a common error on macbooks.

christophermaier commented 2 years ago

@benjyw No, your system isn't unusual... I think mine is 😅 I just ran into a TLS failure along these same lines when a similar change I was making on a work project failed in our CI, which apparently doesn't have TLS 1.3 either. I'm doing some further testing on that codebase to land on the right combination of options to make this more broadly accessible; I'll update this PR once I get something working.

Sorry for the noise! 🙇‍♂️

christophermaier commented 2 years ago

I've relaxed things to TLS 1.2. I'm going to do a bit more research to see if there's a sane way to support folks that may not even have TLS 1.2 support in their curl binaries.

(I left off TLS 1.0 and 1.1 since they're deprecated, but depending on how you want to go, I could relax things even more.)

benjyw commented 2 years ago

Thanks! I think TLS v1.2 is fine to assume - that flag was added to curl 9 in 2013, and TLS 1.2 itself is from 2008.

christophermaier commented 2 years ago

@benjyw Sounds good to me. If 1.2 is fine with you, maybe we can just leave it there. In that case, I think I'm done with changes to this PR.

Thanks!

benjyw commented 2 years ago

Yeah, I think it's fine to require TLS 1.2. If someone really doesn't have support for that, they can always edit the file, this is just the default.

Thanks for the contribution!

Eric-Arellano commented 2 years ago

I think it's fine to require TLS 1.2

Not only fine, but desirable imo. I'm glad for Pants to play a (small) part in making computing more secure. Thank you for your contribution!

If this is a real issue for anyone, they can change the bash script to 1.1.

christophermaier commented 2 years ago

(Updated the PR title for the benefit of your changelog!)