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

Fix TestConnect on macOS #265

Merged
merged 2 commits into from
Jun 1, 2022
Merged

Fix TestConnect on macOS #265

merged 2 commits into from
Jun 1, 2022

Conversation

jdtw
Copy link
Contributor

@jdtw jdtw commented May 31, 2022

@bdd did an excellent writeup of the issue in #264. To fix this, stop relying on stable error messages between platforms and Go versions by performing a successful certigo connect in the test rather than a failed one.

I generated new ECDSA certificates for localhost using https://github.com/square/certstrap.

@jdtw jdtw merged commit 5332ac7 into master Jun 1, 2022
@jdtw jdtw deleted the jwood/fix-TestConnect branch June 1, 2022 17:20
@bdd
Copy link

bdd commented Jun 1, 2022

I was a bit late to chime in with a review before this got merged.
One other source of test failures was the assumption of AES_128_GCM_SHA256 being selected by both the httptest spawned TLS server and the unit test TLS client.

Go alters the default ciphersuite to prefer ChaPoly over AES-GCM suites if the underlying hardware doesn't have AES extensions. (https://cs.opensource.google/go/go/+/master:src/crypto/tls/handshake_server_tls13.go;l=158?q=defaultCipherSuitesTLS13NoAES%20&ss=go%2Fgo). So CI/CD systems running under virtualization may not always reflect the underlying CPUs extensions. One good example—and how I ended up debugging this issue—was NixOS CI system for Darwin x86-64 wasn't picking AES. Temporarily I ended up disabling tests on darwin-x86_64.

So the lines 110 and 138 for expectedConnect string in TestConnect trying to match Cipher Suite: AES_128_GCM_SHA256 cipher will continue causing test failures where Go cannot deduce presence of hardware acceleration for AES.

jdtw added a commit to jdtw/certigo that referenced this pull request Jul 25, 2022
* master: (21 commits)
  Update README.md
  Unconditionally add forward slash in OCSP GET requests (square#282)
  Use Go generate instead of a makefile (square#279)
  Bump github.com/stretchr/testify from 1.7.5 to 1.8.0
  Add support for printing SCTs (square#277)
  Update Go version to 1.18 (square#276)
  Update to 1.16 (square#275)
  Fix OCSP checking (square#274)
  Bump github.com/stretchr/testify from 1.7.2 to 1.7.5
  Update go command to install certigo (square#270)
  Bump github.com/stretchr/testify from 1.7.1 to 1.7.2
  Upgrade yaml.v3 version (square#266)
  Allow any ciphersuite in TestConnect (square#268)
  Fix TestConnect on macOS (square#265)
  Bump github.com/stretchr/testify from 1.7.0 to 1.7.1 (square#262)
  Bump version string to 1.15.1 (square#263)
  Remove use of the format list in the connect command (square#260)
  Add --leaf flag to view the first cert only, including for json and pem
  Set certs explicitly (square#259)
  Upgrade /x/crypto dependency (square#257)
  ...
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.

3 participants