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

using a CN of length >65 throws an idna: invalid label error #11721

Closed
darkedges opened this issue May 28, 2021 · 8 comments
Closed

using a CN of length >65 throws an idna: invalid label error #11721

darkedges opened this issue May 28, 2021 · 8 comments

Comments

@darkedges
Copy link

Describe the bug
When I create a CSR with a Subject DN of UID=f0486469-6aa7-4a93-8cc2-3e64f30e1789,CN=Nicholas Peter Irving it fails to be signed with an error of

idna: invalid label "UID=f0486469-6aa7-4a93-8cc2-3e64f30e1789,CN=Nicholas Peter Irving"

It seems to be checking to see if the CN is less than 64 (it seems to be excluding spaces) characters in length because of this setting
https://github.com/hashicorp/vault/blob/master/builtin/logical/pki/cert_util.go#L790

            p := idna.New(
                idna.StrictDomainName(true),					
   -->       idna.VerifyDNSLength(true),				
            )

The afforementioned code is
https://pkg.go.dev/golang.org/x/net/idna#VerifyDNSLength
https://github.com/golang/net/blob/abc453219eb586afb3fc1742e8c685c28b9f7eea/idna/idna10.0.0.go#L409

Should we be enforcing DNS requirements on the CN, as I am not necessarily creating a TLS Certificate, instead it is a message signing certificate that is not email specific.

Now I have found that if I use exclude_cn_from_sans I can get it signed correctly, but I am unsure what else breaks with that option. I would also have to get the developed of the HashiCorp Java Vault to include that option in their code base.

To Reproduce
Steps to reproduce the behavior:

  1. Create a CSR with a long Subject DN i.e UID=f0486469-6aa7-4a93-8cc2-3e64f30e1789,CN=Nicholas Peter Irving
  2. Submit it for signing

Expected behavior
return a Sign Certificate response.

Environment:

  • Vault Server Version (retrieve with vault status):
    1.5.2
  • Vault CLI Version (retrieve with vault version):
    1.5.2
  • Server Operating System/Architecture:
    Container Image from DockerHub

Vault server configuration file(s):

# Docker default

Additional context
N/A

@darkedges
Copy link
Author

Hi, it has been 5 months now, and we have discovered the workaround is not ideal due to the fact that anything could be present in the CSR and issued.

Could somebody point me in the right direction as to whether we are breaking a standard by asking for > 64 characters, or whether this is a something enforced because most certificates are web based?

@heatherezell
Copy link
Contributor

Hi @darkedges - thanks for your patience. Our engineers are working on researching this further. Appreciate you bringing this back up! :)

@darkedges
Copy link
Author

Thanks @hsimon-hashicorp, appreciated. We could fix it (to our immediate need), but when we go Enterprise we would prefer to have it properly maintained and not have to apply changes each time the product upgrades.

@heatherezell
Copy link
Contributor

Hi @darkedges! I spoke to an expert on this, and he states that exclude_cn_from_sans is specifically for your scenario, and does not make any other changes. The specific cause of the error you're seeing isn't due to the CN length, it's due to the DNS-invalid characters provided in the CN. We'd be happy to discuss use cases and other information with you, though! Please let me know if this helps, or if you have more questions. :)

@darkedges
Copy link
Author

Hi @hsimon-hashicorp IIRC that option only is useful if the CN an @ symbol otherwise it thinks it is an DNS name and apply the rules. Have a look at the code and you will see what I mean.

@heatherezell
Copy link
Contributor

Hi @darkedges! I took a look at the code:

if cn != "" && !data.apiData.Get("exclude_cn_from_sans").(bool) {
if strings.Contains(cn, "@") {
// Note: emails are not disallowed if the role's email protection
// flag is false, because they may well be included for
// informational purposes; it is up to the verifying party to
// ensure that email addresses in a subject alternate name can be
// used for the purpose for which they are presented
emailAddresses = append(emailAddresses, cn)
} else {
// Only add to dnsNames if it's actually a DNS name but convert
// idn first
p := idna.New(
idna.StrictDomainName(true),
idna.VerifyDNSLength(true),
)
converted, err := p.ToASCII(cn)
if err != nil {
return nil, errutil.UserError{Err: err.Error()}
}
if hostnameRegex.MatchString(converted) {
dnsNames = append(dnsNames, converted)
}
}
}
and it appears that the rules only get applied if that option is not specified. Is there another code block you're thinking of? Thanks! :)

@heatherezell heatherezell removed the bug Used to indicate a potential bug label Oct 22, 2021
@darkedges
Copy link
Author

darkedges commented Oct 22, 2021

Thanks @hsimon-hashicorp, I have reviewed my test case again and worked out the issue after testing on 1.5.2, 1.7.3 and 1.8.3. and now believe it was a misunderstanding on my behalf around
a) the Common Name field
b) the time between comments.

I think first I wanted to know what else could be broken if I used the exclude_cn_from_sans, as I did not want it to break anything else. No upon reflection it is that misunderstanding about Common Name that leads to the real issue about CSR and the Subject field that I will raise a seperate issue about.

HC Vault is only allowing CN in that field, unless you use Sign Verbatim, which is not recommended to use. So if I generate a CSR via

openssl req -new -newkey rsa:2048 -nodes -out star_example_com.csr -keyout star_example_com.key -subj "/C=AU/ST=Victoria/L=Melbourne/O=DarkEdges/OU=IT/CN=*.example.com"

the certficate generated will only contain CN=*.example.com and that is the fundamental issue.

I have raised #12912 to get that feature added.

Will close this issue.

@darkedges
Copy link
Author

darkedges commented Oct 22, 2021

Closing and moving to feature request

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

No branches or pull requests

3 participants