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

X509 Certificate Validation - x5t Thumbprint #35

Merged
merged 25 commits into from
Nov 24, 2021
Merged

Conversation

nenaraab
Copy link
Contributor

@nenaraab nenaraab commented Nov 12, 2021

Provides Certificate Thumbprint (x5t) validator which becomes part of a set of proof of possession validators.

  1. Prerequisite: the token must provide a cnf claim with x5t#S256 member.

    Example:
    "cnf": {
      "x5t#S256": "2blWiJDH07q0b2qEwShOIxtt10CkZ5xdDw4Vbs8ddoI"
    }
  2. For that validation step a SHA-256 thumbprint of the client certificate RFC 8705 needs to be created.
  3. To check whether my application is a legit target for this SHA-256 thumbprint needs to match with the cnf token claim.
  4. Furthermore it provides a auth.ClientCertificateFromCtx(request) to get a x509.Certificate if provided within the x-forwarded-client-cert header.
  5. It supports DER and PEM formatted certificates.
  6. Instead of middleware.Authenticate use AuthenticateWithProofOfPossession, which returns
  • instead of two parameters: token and err
  • three parameters, namely token, *x509.Certificate and err

@nenaraab nenaraab requested a review from a team as a code owner November 12, 2021 15:58
@nenaraab nenaraab added do-not-merge enhancement New feature or request labels Nov 12, 2021
@nenaraab nenaraab requested a review from liga-oz November 15, 2021 16:32
Copy link
Contributor

@liga-oz liga-oz left a comment

Choose a reason for hiding this comment

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

Just a few remarks to consider, overall looking good 👍🏻

auth/proofOfPossession.go Outdated Show resolved Hide resolved
auth/proofOfPossession_test.go Outdated Show resolved Hide resolved
auth/proofOfPossession_test.go Outdated Show resolved Hide resolved
auth/token.go Show resolved Hide resolved
auth/proofOfPossession_test.go Outdated Show resolved Hide resolved
Copy link
Member

@f-blass f-blass left a comment

Choose a reason for hiding this comment

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

I like your table tests 👍

Added however some remarks and questions.

auth/middleware.go Outdated Show resolved Hide resolved
auth/proofOfPossession.go Outdated Show resolved Hide resolved
auth/proofOfPossession.go Outdated Show resolved Hide resolved
auth/middleware.go Show resolved Hide resolved
auth/middleware.go Outdated Show resolved Hide resolved
auth/middleware.go Show resolved Hide resolved
auth/proofOfPossession.go Outdated Show resolved Hide resolved
auth/token.go Outdated Show resolved Hide resolved
auth/token.go Outdated Show resolved Hide resolved
@nenaraab nenaraab requested review from f-blass and liga-oz November 17, 2021 17:53
liga-oz
liga-oz previously approved these changes Nov 18, 2021
Copy link
Contributor

@liga-oz liga-oz left a comment

Choose a reason for hiding this comment

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

approved 😄 I hope you checked the fwd-cert header format coming from k8s

Copy link
Member

@f-blass f-blass left a comment

Choose a reason for hiding this comment

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

LGTM!

I have just a few comments mainly in speeding up the tests.

}

func generateCert(t *testing.T, pemEncoded bool) string {
key, err := rsa.GenerateKey(rand.Reader, 2048)
Copy link
Member

Choose a reason for hiding this comment

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

Based on the PR from Jakob yesterday. Does it make sense to cache this cert or do you actually need new certs each test run? Maybe the key can also be shortened to 512 bits.

auth/proofOfPossession_test.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
auth/proofOfPossession.go Outdated Show resolved Hide resolved
f-blass
f-blass previously approved these changes Nov 24, 2021
@nenaraab nenaraab merged commit 7c1e248 into master Nov 24, 2021
@nenaraab nenaraab deleted the cert_validation branch November 24, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants