PeculiarVentures / PKI.js

PKI.js is a pure JavaScript library implementing the formats that are used in PKI applications (signing, encryption, certificate requests, OCSP and TSP requests/responses). It is built on WebCrypto (Web Cryptography API) and requires no plug-ins.
http://pkijs.org
Other
1.3k stars 204 forks source link

Subject DN in wrong format #169

Closed mourkeita closed 6 years ago

mourkeita commented 6 years ago

Hello, When i generate CSR with PKIjs, the Subject DN in a CSR is in a wrong format : fields are delimited with "+" instead of "," (see screenshots). Can you please help me to transform the subject DN generation to this format (delimited with "," commas).

Thanks

good subject subject dn

YuryStrozhevsky commented 6 years ago

Check this issue and all linked issues there. It is a kind of feature of PKIjs. You would need to implement your own code if you would like to "fix" this.

microshine commented 6 years ago

@mourkeita this help function changes DN https://github.com/PeculiarVentures/fortify-web/blob/master/src/helpers/cert_helper.js#L98

mourkeita commented 6 years ago

Thanks @microshine ! This was very helpful. 😄

YuryStrozhevsky commented 6 years ago

@mourkeita Seems you missed my sentence:

Check this issue and all linked issues there

You could check that at the end of this issue's thread there is a link to this pull request with a possible code.

@microshine Instead of making "a fix functions" always better to use simple re-definition just for necessary function in major class. This is how object-oriented programming concept works.

import RelativeDistinguishedNames from "pkijs/src/RelativeDistinguishedNames.js";
import CertificationRequest from "pkijs/src/CertificationRequest.js";
import AttributeTypeAndValue from "pkijs/src/AttributeTypeAndValue.js";
//*******************************************************************************************
class AnotherRelativeDistinguishedNames extends RelativeDistinguishedNames
{
    constructor(parameters)
    {
        super(parameters);
    }

    toSchema()
    {
        //region Decode stored TBS value
        if(this.valueBeforeDecode.byteLength === 0) // No stored encoded array, create "from scratch"
        {
            return (new asn1js.Sequence({
                value: Array.from(this.typesAndValues, element => (
                    new asn1js.Set({
                        value: [element.toSchema()]
                    })
                ))
            }));
        }

        const asn1 = asn1js.fromBER(this.valueBeforeDecode);
        //endregion

        //region Construct and return new ASN.1 schema for this object
        return asn1.result;
        //endregion
    }
}
//*******************************************************************************************
function encodePKCS10()
{
    const pkcs10 = new CertificationRequest();

    pkcs10.subject = new AnotherRelativeDistinguishedNames();
    pkcs10.version = 0;
    pkcs10.subject.typesAndValues.push(new AttributeTypeAndValue({
        type: "2.5.4.6",
        value: new asn1js.PrintableString({ value: "RU" })
    }));
    pkcs10.subject.typesAndValues.push(new AttributeTypeAndValue({
        type: "2.5.4.3",
        value: new asn1js.Utf8String({ value: "Simple test (простой тест)" })
    }));

    pkcs10.attributes = [];

    const buffer = pkcs10.toSchema(true).toBER(false);
}
//*******************************************************************************************

Such class you could use anywhere as a replacement for major class if you wish so.

But in all cases I want to say that there is no mistakes in existing encoding of RelativeDistinguishedNames class - all is encoded 100% correct. The LDAP-like representation of relative name ("CN=blah-blah, O=blah-blah" etc.) is a secondary representation and nothing stops developers to use CN=blah-blah, O=blah-blah instead of "CN=blah-blah+O=blah-blah" - it is only a will of developer how to represent name.

mourkeita commented 6 years ago

Thanks @YuryStrozhevsky for your usefull post. :)

microshine commented 6 years ago

AWS certlint and zlint create Warning if subject name has more than one RDN

https://github.com/awslabs/certlint/blob/25d5957f8c36dafcbd82870df57a8367b49650be/lib/certlint/namelint.rb#L121

https://github.com/zmap/zlint/blob/master/lints/lint_subject_multiple_rdn.go#L42

Maybe it would be better to add AnotherRelativeDistinguishedNames to PKIjs and use it by default

YuryStrozhevsky commented 6 years ago

@microshine Would be better to create issues for certlint and zlint where describe their errors here. As a template could be used this issue for Go language.

YuryStrozhevsky commented 6 years ago

Also seems in these packages there is misunderstanding in basic concepts. The problem is that there is no such entity as RelativeDistinguishedName (RDN). There is only RelativeDistinguishedNames (pay attention on ...Names) - a set of name attributes. So, it is wrong to say there are many RDNs or there is single RDN - there are only different representations of name attributes and that is all. Again - there is no such entity as RDN, there is only a set of attributes with name RelativeDistinguishedNames.

YuryStrozhevsky commented 6 years ago

@microshine Just for reference I need to say that seems I was not 100% correct in my least comment: there is a definition of the RelativeDistinguishedName. Here is how RelativeDistinguishedNames described in ASN.1:

RDNSequence ::= SEQUENCE OF RelativeDistinguishedName

RelativeDistinguishedName ::=
SET SIZE (1..MAX) OF AttributeTypeAndValue

In currect PKIjs code we have:

Sequence 
{
   Set({
     attribute1,
     attribute2,
     .....
   })
}

In fact as you can see here there is only single RelativeDistinguishedName.

In the AnotherRelativeDistinguishedNames we would have:

Sequence 
{
  Set(attribute1),
  Set(attribute2),
  .....
}

And here we would have many RelativeDistinguishedName values.

But both packages make warnings not on RDN count but on multiple attribute values inside one RDN and this is strangefor me. I will write issues to both packages you mentioned and try to ask them for explanation on these warnings.

microshine commented 6 years ago

@YuryStrozhevsky thank you

YuryStrozhevsky commented 6 years ago

@microshine So, zlint would be changed. As well as most probably certlint.

rmhrisk commented 6 years ago

While it is true that Zlint will be changed it seems this check was added to this and cablint so facilitate interoperability with poorly written libraries like the Ruby OpenSSL binding.

We should consider if this representation is the right one to default to even if it is correct.

sarim commented 1 year ago

Sorry for necro bumping. But thought I should share my exeprience for future googler and future me. I was importing a root certificate generated by mkcert, then generating new certificate using that cert, I was setting my cert's issuer by reading root cert's subject, unfortunately, mkcert had them in separate sets, and my cert's issuer had them in one set. So both chrome and curl was complaining that authority is invalid. Had to apply a similar workaround in https://github.com/PeculiarVentures/PKI.js/issues/169#issuecomment-384174083 .

~(I should just get and set binary format, but I currently can't due another issue with ArrayBuffer implementation in njs)~

Update

I just checked both cloudflare and lets encrypt. Both of their certificate is using seperate set, aka comma between fields. So I guess the separate set became the industry standard.

↪ ~ ➤ openssl s_client -connect cloudflare.com:443 -showcerts </dev/null 2>/dev/null | openssl x509 -text -noout | grep Issuer:
        Issuer: C = US, O = "Cloudflare, Inc.", CN = Cloudflare Inc ECC CA-3
↪ ~ ➤ openssl s_client -connect letsencrypt.org:443 -showcerts </dev/null 2>/dev/null | openssl x509 -text -noout | grep Issuer:
        Issuer: C = US, O = Let's Encrypt, CN = R3

Update 2

certificate.issuer = pkijs.RelativeDistinguishedNames.fromBER(issuerCertificate.subject.toSchema().toBER(false))
// OR
certificate.issuer = issuerCertificate.subject;

throws AsnError: Error during parsing of ASN.1 data. Data is not correct for 'RelativeDistinguishedNames' I'm guessing this is because pkijs's expecting different formatted RDN

So I'll keep using my workaround: looping over issuerCertificate.subject.typesAndValues.forEach and add values from scratch along with my SeperatedRelativeDistinguishedNames subclass.