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

Update x509.h #24243

Closed
wants to merge 1 commit into from
Closed

Update x509.h #24243

wants to merge 1 commit into from

Conversation

ducksinc
Copy link

Issue #20663

CSR v1 certificates should always be used. If other versions are used, it will result in CSRs that are invalid. Therefore, all instances of setting the version were changed to use version 0, always.

CLA: trivial

Checklist
  • documentation is added or updated
  • tests are added or updated

Issue openssl#20663

CSR v1 certificates should always be used. If other versions are used,  it will result in CSRs that are invalid.
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Apr 23, 2024
@ducksinc
Copy link
Author

@bbbrumley
I took on an issue where certificate signing requests were able to use different versions, even though only one version worked. I hardcoded all instances of the version to use 0, which is CSR v1 based on RFC 2986, and the only version that works.

@@ -62,7 +68,7 @@ struct x509_sig_info_st {

struct X509_req_info_st {
ASN1_ENCODING enc; /* cached encoding of signed part */
ASN1_INTEGER *version; /* version, defaults to v1(0) so can be NULL */
int ASN1_INTEGER = 0; /* CSR v1 will always be used */
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not valid C.

ASN1_INTEGER is a type, not the name of a structure member, and you can't give default values like this. In addition, changing from a pointer to an integer type would be a change of API (even if this was valid C).

@bbbrumley
Copy link
Contributor

@ducksinc thanks for identifying this open issue

Some other gripes with the PR, but high level .. isn't Issue #20663 solved by PR #23965 ? (Maybe the Issue was errantly left open?)

@t8m
Copy link
Member

t8m commented Apr 23, 2024

Yes, @bbbrumley is right. This was already fixed.

@t8m t8m closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold: cla required The contributor needs to submit a license agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants