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

Support loading of PEM-encoded OCSP Responses #79

Closed

Conversation

robstradling
Copy link
Contributor

I'm looking for a way to lint PEM-encoded OCSP Responses, which pkilint currently rejects.

A quick search suggests that -----BEGIN OCSP RESPONSE----- and -----END OCSP RESPONSE----- are used by several other projects.

@CBonnell I realise that RFC7468 Section 4 doesn't define a standard PEM encapsulation boundary for OCSP responses, but would you be willing to accept this PR anyway?

@CBonnell
Copy link
Collaborator

CBonnell commented Jun 4, 2024

I think this is a good idea to create parity with the other document type REST API endpoints. Before merging, let me check with one of the consumers of the REST API to ensure this change doesn't introduce any breaking changes for them.

@CBonnell
Copy link
Collaborator

Since this seemingly will not pose any compatibility issues for current users, I plan on including this in the next release (probably within the next two weeks).

@robstradling
Copy link
Contributor Author

Thanks @CBonnell. Hmm, test failure. Not sure why. :-|

Just FYI, I'm planning to open-source pkimetal next week, and its OCSP response linting API won't work until this PR lands.

@CBonnell
Copy link
Collaborator

I think the PEM load function for OCSP responses is now failing, as it previously was expecting base64 and with this change it's expecting PEM (with the ASCII armor).

Will it cause any issues on the pkimetal side if, for OCSP responses only, both base64 and PEM are accepted?

@robstradling
Copy link
Contributor Author

That would be fine from my side. Thanks Corey.

@robstradling
Copy link
Contributor Author

@CBonnell Do you need me to do that in this PR? (If so, some tips would be appreciated! :-) )

@CBonnell
Copy link
Collaborator

I'll work on the document loader to accept both base64 and PEM in a separate PR. Once that's in place, (hopefully) this PR can be merged with no additional changes.

@robstradling
Copy link
Contributor Author

Great. Thanks @CBonnell !

@CBonnell
Copy link
Collaborator

CBonnell commented Jul 1, 2024

@robstradling, I went ahead and significantly reworked the loader module: https://github.com/digicert/pkilint/pull/85/files#diff-1da309f0c48ca7f0db9304ba7aed66c482ecbdcafe66d430c30f295ecab94630. As part of this refactoring, the "OCSP RESPONSE" PEM label is now supported. Thank for you this suggestion, it certainly helps to add parity to the loader interface across document types.

Given this refactoring, I don't believe this PR is needed anymore. I'll create a new issue to document your original suggestion so this enhancement is noted in the CHANGELOG. Let me know if you have any problems with this approach.

@robstradling
Copy link
Contributor Author

Thanks @CBonnell. That sounds great!

@CBonnell
Copy link
Collaborator

CBonnell commented Jul 2, 2024

Closed in favor of the refactoring of the loader module. This suggestion is documented in #86.

@CBonnell CBonnell closed this Jul 2, 2024
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.

2 participants