Skip to content

Allow extra content in CSR file for ReadCertificateRequest#488

Merged
dopey merged 8 commits intomasterfrom
max/readCertificateRequest
Apr 22, 2024
Merged

Allow extra content in CSR file for ReadCertificateRequest#488
dopey merged 8 commits intomasterfrom
max/readCertificateRequest

Conversation

@dopey
Copy link
Copy Markdown
Contributor

@dopey dopey commented Apr 16, 2024

No description provided.

@dopey dopey requested a review from maraino April 16, 2024 04:43
Copy link
Copy Markdown
Contributor

@maraino maraino left a comment

Choose a reason for hiding this comment

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

At least we need to support the errors.Is method, but we should be adding your changes to other related methods in this package.

Comment thread pemutil/pem.go Outdated
Comment thread pemutil/pem.go Outdated
Comment thread pemutil/pem.go Outdated
Comment thread pemutil/pem.go Outdated
Comment thread pemutil/pem.go
Comment thread pemutil/pem.go Outdated
Comment on lines +320 to +323
if e.Type == PEMTypeUndefined {
return fmt.Sprintf("file %s does not contain valid PEM encoded data", e.File)
}
return fmt.Sprintf("file %s does not contain a valid PEM encoded %s", e.File, e.Type)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It works for now, but the file might not be present if we add this error to methods like ParseCertificate where the arguments are just the PEM bytes.

Comment thread pemutil/pem.go Outdated
Comment on lines +415 to +416
return csr, &InvalidPEMError{File: filename, Type: PEMTypeCertificateRequest,
Err: fmt.Errorf("error parsing %s: CSR PEM block is invalid: %w", filename, err)}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would make more sense to set that as the message and set Err: err

@dopey dopey requested a review from maraino April 19, 2024 04:10
Copy link
Copy Markdown
Contributor

@maraino maraino left a comment

Choose a reason for hiding this comment

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

Just an small change

Comment thread pemutil/pem.go Outdated
@dopey dopey marked this pull request as ready for review April 22, 2024 21:01
Comment thread pemutil/pem.go Outdated
Co-authored-by: Mariano Cano <[email protected]>
@dopey dopey requested a review from maraino April 22, 2024 21:07
Copy link
Copy Markdown
Contributor

@maraino maraino left a comment

Choose a reason for hiding this comment

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

lgtm

@dopey dopey merged commit 03fcfce into master Apr 22, 2024
@dopey dopey deleted the max/readCertificateRequest branch April 22, 2024 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants