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

Fix requirements on creating certificates #11312

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ahaczewski
Copy link

Changes:

TrueNAS allows creating CA certificates and CSRs. These can be used for internal network purposes, but a CSR from TrueNAS is also a way to get ACME-DNS authorized certificate from Let's Encrypt.

Let's Encrypt accepts only two fields in a CSR: Common Name, and a list of SANs. All the other subject fields are stripped from the generated certificate, so it makes no sense to require for a certificate to have a country, state, locality and organization.

Additionally, when creating a CA certificate, requiring SANs makes no sense, as these are to be used for the leaf certificates.

Common Name, on the other hand, should be required for all kinds of certificates.

Testing:

  • Create certificate authority with only Common Name filled
  • Create CSR with only Common Name and SANs filled

Downstream

Affects Reasoning
Documentation Not affected, as it does not mention the fields are required. I would suggest thou to mention the required field Common Name.

TrueNAS allows creating CA certificates and CSRs. These can be used for
internal network purposes, but a CSR from TrueNAS is also
a way to get ACME-DNS authorized certificate from Let's Encrypt.

And Let's Encrypt accepts only two fields in a CSR: Common Name, and a list of SANs.
All the other subject fields are stripped from the generated certificate,
so it makes no sense to require for a certificate to have a country,
state, locality and organization.

Additionally, when creating a CA certificate, requiring SANs makes no sense,
as these are to be used for the leaf certificates.

Common Name, on the other hand, should be required for all kinds of certificates.
@ahaczewski ahaczewski requested a review from a team as a code owner January 8, 2025 19:40
@ahaczewski ahaczewski requested review from undsoft and removed request for a team January 8, 2025 19:40
},
]);
});

it('allows creating Lets Encrypt-compatible subject', async () => {
Copy link
Author

@ahaczewski ahaczewski Jan 8, 2025

Choose a reason for hiding this comment

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

This test fails, because I have no idea how to force IxSelectHarness (for Country) to clear itself of selected value. I would happily fix this if someone would be so nice to chime in and lend me a hand 😃

@ahaczewski
Copy link
Author

ahaczewski commented Jan 8, 2025

The PR at its current state will fail the build, because the test I created fails. More on that in the in-line comment. I could use some help with that 😊

Copy link
Collaborator

@undsoft undsoft left a comment

Choose a reason for hiding this comment

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

Unfortunately, this won't work because API has validation in place for city, email, organization and state fields.

@themylogin Would you like to chip in on the discussion?

@themylogin
Copy link
Contributor

@sonicaj @Qubad786 will it be possible to disable validation for these fields in case of Let's Encrypt?

@ahaczewski
Copy link
Author

Unfortunately, this won't work because API has validation in place for city, email, organization and state fields.

@themylogin Would you like to chip in on the discussion?

I'm happy to remove validation there as well, just need to find it in the API first 😅

@sonicaj @Qubad786 will it be possible to disable validation for these fields in case of Let's Encrypt?

But why only for Let's Encrypt? Certificate Authorities do not have e-mail nor SANs, check GitHub, Google, Amazon, Cloudflare or any other root certificate.

@sonicaj
Copy link
Member

sonicaj commented Jan 13, 2025

@ahaczewski i have created a ticket here https://ixsystems.atlassian.net/browse/NAS-133533 which will cater to the points you have raised, the changes on the UI side alone won't be enough to account for the points raised and middleware (truenas/middleware repo) will have to make some changes to get this to work.

@ahaczewski
Copy link
Author

@sonicaj Awesome! If nobody will jump in on the middleware, I'll try to carve some time over the weekend and give it a try myself 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants