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

OCSP integrationv2 test with GnuTLS #3207

Merged
merged 44 commits into from
Mar 29, 2022
Merged

OCSP integrationv2 test with GnuTLS #3207

merged 44 commits into from
Mar 29, 2022

Conversation

goatgoose
Copy link
Contributor

@goatgoose goatgoose commented Feb 22, 2022

Resolved issues:

resolves #2140

Description of changes:

Added tests to check that s2n clients accept OCSP responses from servers, and that clients accept OCSP responses from an s2n server. Adds a GnuTLS provider that's used in this test.

Additionally added GnutTLS to other existing tests:

  • happy path
  • signature algorithms
  • fragmentation
  • version negotiation

Call-outs:

N/A

Testing:

Codebuild test: https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/SingleIntegrationV2/build/SingleIntegrationV2%3A1e2381bc-a68a-4256-8525-b581bd8757b0

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dougch
Copy link
Contributor

dougch commented Feb 22, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 22, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 25, 2022

Benchmark report
No change in performance detected.

@goatgoose goatgoose marked this pull request as ready for review February 25, 2022 16:26
@goatgoose goatgoose requested a review from dougch as a code owner February 25, 2022 16:26
@dougch
Copy link
Contributor

dougch commented Feb 25, 2022

Benchmark report
No change in performance detected.

Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

Looks good, a few questions. Liking the formatting and variable name cleanup.

S2N_INTEG_TEST=1 \
PATH="../../bin":$(LIBCRYPTO_ROOT)/bin:$(PATH) \
PATH="$(S2N_ROOT)/bin":"$(S2N_ROOT)/test-deps/gnutls37/bin":"$(LIBCRYPTO_ROOT)/bin":$(PATH) \
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, relative paths are insidious

Signatures.RSA_SHA512: "SIGN-RSA-SHA512",

# GnuTLS only supports this signature in TLS 1.1
# Signatures.RSA_SHA224: "SIGN-RSA-SHA224",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe an overall summary comment here about where these came from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these comments because the other tests were using these signature algorithms. However, the v1 tests don't use them, so I just removed the comments for now: 5f221b0

)

if provider == GnuTLS:
# GnuTLS fails to validate the certificates. must run test in insecure mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Known bug or issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this seems weird, I don't know why Gnutls wouldn't be able to verify our certs. We probably need to at least know why this is the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get the following errors when running this without the insecure flag:

  • issuer in verification was not found or insecure; trying against trust list
  • Fatal error: Error in the certificate.

Could this be an issue with self signing? I switched the insecure flag with a --no-ca-verification flag: 10037a4. With this, the tests passed.

Openssl s_client has a setting that’s not enabled by default: -verify_return_error. From the docs:
Return verification errors instead of continuing. This will typically abort the handshake with a fatal error.

I added this flag, and the openssl client also aborted the handshake with the error: Verify return code: 19 (self signed certificate in certificate chain).

Could gnutls just be more strict than openssl by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay gnutls not liking self-signed certs makes sense, I prefer the --no-ca-verification flag since it's more specific than 'insecure'.

@dougch dougch requested a review from maddeleine March 10, 2022 22:41
)

if provider == GnuTLS:
# GnuTLS fails to validate the certificates. must run test in insecure mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this seems weird, I don't know why Gnutls wouldn't be able to verify our certs. We probably need to at least know why this is the case.

@dougch dougch added the s2n-core team label Mar 18, 2022
@camshaft camshaft enabled auto-merge (squash) March 29, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OCSP Integration test
4 participants