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

feat(inputs.x509_cert): Add OCSP stapling information for leaf certificates (#10550) #12444

Merged
merged 5 commits into from
Feb 17, 2023

Conversation

jjh74
Copy link
Contributor

@jjh74 jjh74 commented Dec 29, 2022

Required for all PRs

resolves #10550

This PR adds OCSP stapling information from tls connectionstate(https://pkg.go.dev/crypto/tls#ConnectionState) (https://pkg.go.dev/golang.org/x/crypto/ocsp#ParseResponse). OCSPResponse signature is not checked against issuer certificate.

New fields: ocsp_status(Good=0,Revoked=1,Unknown=2), ocsp_produced_at, ocsp_this_update, ocsp_next_update and ocsp_revoked_at (when ocsp_status=2).
New tags: ocsp_stapled yes/no

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Dec 29, 2022
@powersj powersj assigned powersj and unassigned powersj Jan 26, 2023
@srebhan
Copy link
Contributor

srebhan commented Jan 27, 2023

@jjh74 can you please run make tidy to make CI happy!?

@srebhan srebhan self-assigned this Jan 27, 2023
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

@jjh74 thanks for looking into this! I have one concern regarding the assumed order of certificates in the answer returned by the server. Currently, we assume (and also you do it) that we always receive the leaf certificate as the first entry. However, I do not see any guarantee for that order, neither by RFC2459 nor by RFC5280 nor by the Golang code.
So IMO we need to order the certs, i.e. construct the trust-path(s)/tree and then determine the leaf nodes instead of blindly use index zero...

What do you think?

plugins/inputs/x509_cert/x509_cert.go Outdated Show resolved Hide resolved
plugins/inputs/x509_cert/x509_cert.go Outdated Show resolved Hide resolved
plugins/inputs/x509_cert/x509_cert.go Outdated Show resolved Hide resolved
plugins/inputs/x509_cert/x509_cert.go Outdated Show resolved Hide resolved
Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Nice update @jjh74! Only two small comments left. Please also resolve the merge conflict...

go.mod Outdated Show resolved Hide resolved
plugins/inputs/x509_cert/x509_cert.go Outdated Show resolved Hide resolved
@srebhan
Copy link
Contributor

srebhan commented Feb 9, 2023

@jjh74 any news on this PR?

@jjh74
Copy link
Contributor Author

jjh74 commented Feb 9, 2023

@jjh74 any news on this PR?

I tried to do the go.mod require merge + moved import "golang.org/x/crypto/ocsp". I hope I got it right.
(circleci complains about make check-deps (LICENSE_OF_DEPENDENCIES.md), is this something I should try to fix ?)

I realized that ocsp.ParseResponse(*ocspresp, nil) doesn't verify the ocsp stapling response signature against issuer certificate (https://pkg.go.dev/golang.org/x/crypto/ocsp#ParseResponse). I can try to see if it's possible to figure out issuer cert and verify ocsp stapling response or if this PR looks good now I can work on ocsp response verify on different PR ?

@srebhan
Copy link
Contributor

srebhan commented Feb 9, 2023

Yeah I noticed that. It seems like you are adding those new libs as dependencies. Maybe try to rebase on latest master to fix the merge conflicts and see if the issue is gone. If not, you need to add the two entries to the docs/LICENSE_OF_DEPENDENCIES.md file...

@jjh74
Copy link
Contributor Author

jjh74 commented Feb 13, 2023

Yeah I noticed that. It seems like you are adding those new libs as dependencies. Maybe try to rebase on latest master to fix the merge conflicts and see if the issue is gone. If not, you need to add the two entries to the docs/LICENSE_OF_DEPENDENCIES.md file...

I rebased to master and that seemed to work.

After rebase I added two more commits:

  1. If server sends certificate chain, try to figure out issuer certificate and verify ocsp stapling response with issuer cert. If verification fails retry ocsp.ParseResponse w/out issuer cert.
  2. Updated README.md with ocsp tags/fields (run-readme-linter seems to fail (but AFAIK on different plugin/README))

jjh74 and others added 5 commits February 16, 2023 10:18
…icates (influxdata#10550)

This adds OCSPResponse information from tls connectionstate for leaf certificates.

Fields: ocsp_status_code(Good=0,Revoked=1,Unknown=2), ocsp_produced_at, ocsp_this_update, ocsp_next_update
and ocsp_revoked_at (when ocsp_status=2).
Tags: ocsp_stapled yes/no, ocsp_status good/revoked/unknown
ocspreponse. If verification fails retry ocsp.ParseResponse w/out
issuer certificate.
@powersj
Copy link
Contributor

powersj commented Feb 16, 2023

@jjh74 I pushed a few commits to your PR. Namely, I rebased on master and fixed the conflicts and one linter issue. This should hopefully get us nearly there. I can do a review in a bit.

@telegraf-tiger
Copy link
Contributor

@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 16, 2023
Copy link
Contributor

@srebhan srebhan 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 to me. Thanks @jjh74 for the contribution and thanks @powersj for driving this over the finish line.

@srebhan srebhan merged commit 54c0919 into influxdata:master Feb 17, 2023
@xoxys
Copy link

xoxys commented Feb 17, 2023

Thanks everyone for the implementation!

@jjh74 jjh74 deleted the feat_issue_10550 branch March 22, 2023 12:46
@srebhan srebhan added this to the v1.26.0 milestone Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[inputs.x509_cert] detect revoked certificates
4 participants