-
Notifications
You must be signed in to change notification settings - Fork 580
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
chore(deps): upgrade goproxy 1.7.0 #5739
base: main
Are you sure you want to change the base?
Conversation
d0f4ea7
to
fddad70
Compare
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.
Looks good so far! So if I understand correctly, in InitCA
,
- rootCAs are the trusted root certs that the proxy needs to make outbound connections on behalf of the proxied CLI
- certPEMBlock is the self-signed proxy CA that holds the root of trust for the proxied CLI
So it makes sense that the proxy's CA wouldn't need the extra CA certs -- the CLI already trusts the self-signed one! Great catch!
For a test, I would suggest re-creating two scenarios if you want to be really bulletproof here. Both will require a standalone test TLS server with its certificate issued by a self-signed CA -- but a completely separate one from the proxy's CA.
- In one scenario, add the test CA cert (that issued the test TLS server's cert) to the cert pool. It's currently x509.SystemCertPool, which will affect all tests running in the same process, so you might want to patch a separate pool for this test if this becomes a problem.
- In another scenario, add the test CA cert (that issued the test TLS server cert) to the file at SNYK_CA_CERTIFICATE_LOCATION_ENV.
In both scenarios, it should be possible to interact with the test TLS server (simple GET request is fine) through the proxy without disabling cert verification.
if extraCertificateError == nil { | ||
// add to pem data | ||
certPEMBlock = append(certPEMBlock, '\n') |
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.
Question: Is this really correct to do? Since the test fails I would say no.
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.
In discussion we are going to split the certPEMBlock, at the moment we are using this artefact for two purposes.
- Configuring the Node.js
- Setting up the proxy
7437cd3
to
02d3250
Compare
This now looks better now! Let's think a bit more about extending the test cases. Maybe we can create one that would have triggered the bug we are fixing now. |
02d3250
to
462983c
Compare
Pull Request Submission Checklist
What does this PR do?
This change indicates that the system now only keeps the original self-signed certificate in the PEM block while still adding additional CA certificates to the certificate pool via rootCAs.AddCert(). This is a more efficient approach since:
This aligns with the behaviour implemented in goproxy up until this change
How should this be manually tested?
You will need:
Instructions assume MacOS, adjust for your platform as required.
Original bug as seen with v1.1295.1
curl --compressed https://downloads.snyk.io/cli/v1.1295.1/snyk-macos -o ~/snyk-1.1295.1
export NODE_EXTRA_CA_CERTS='/usr/local/etc/openssl@3/cert.pem'
~/snyk-1.1295.1 test .
write EPROTO 0052E60602000000:error:0A000098:SSL routines:read_state_machine:excessive message size:../deps/openssl/openssl/ssl/statem/statem.c:621:
Verify the fix
export NODE_EXTRA_CA_CERTS='/usr/local/etc/openssl@3/cert.pem'
./binary-releases/snyk-macos test .
Reference: CLI-658