Created attachment 476672 [details]
Patch to test.

Dimitri, make sure you have the right people who can test proposed patches CC'ed here.


bob

Since RHEL 6.1 External Beta has begun, and this bug remains
unresolved, it has been rejected as it is not proposed as
exception or blocker.

Red Hat invites you to ask your support representative to
propose this request, if appropriate and relevant, in the
next release of Red Hat Enterprise Linux.

Since RHEL 6.2 External Beta has begun, and this bug remains
unresolved, it has been rejected as it is not proposed as
exception or blocker.

Red Hat invites you to ask your support representative to
propose this request, if appropriate and relevant, in the
next release of Red Hat Enterprise Linux.

Dmitri, has anyone from your team ever tested the patch that Bob had attached, to see if it satisfies your requirements?
If not, should we make a scratch build for you for testing?

Created attachment 740648 [details]
original unified patch

The attached patch wasn't (!) a unified diff.

I had to checkout the original CVS versions of the modified files - which were luckily mentioned in the patch file, applied the patch, and created a unified diff with context.

Created attachment 740651 [details]
patch v2

I've merged the patch to the tip of NSS (3.15).

I've submitted a scratch build. based on NSS 3.14.3 for RHEL 6, which includes the patch from this bug. Because it was easier for me to avoid merging, I've also included the patch from mozilla 757857 (which adds name constraints support).

Dmitri, could you please get the scratch build, and give feedback if this patch works for you? Thanks.

https://brewweb.devel.redhat.com/taskinfo?taskID=5696455
(hopefully build will pass, if not, I'll do another one tomorrow.)

The scratch build failed, because the test suite failed.

The test suite failed several times. I've looked at the first failure, only.

We crash in function AddSubjectAltNames, because it calls PORT_Strdup(NULL), which isn't allowed.

I've tried the obvious fix, by changing the beginning of function AddSubjectAltNames.
I've changed
    char *names = PORT_Strdup(constNames);
to
    char *names = NULL;
    
    if (constNames)
	names = PORT_Strdup(constNames);
  
With that change, we no longer crash, however, the certutil command fails with an error message.

In order to see the failure, use an existing tests_results output, go to the "eve" subdirectory, and run the following command:

certutil -C -c TestCA -m 2060 -v 60 -d ../CA -i req -o Eve.cert -f ../tests.pw -7 eve,eve,beve

Without this patch, NSS succeeds.

With the patch applied, certutil prints:

certutil: Problem creating SubjectAltName extension: Certificate extension not found.
certutil: unable to create cert (Certificate extension not found.)

Created attachment 740860 [details]
patch v4 (on top of NSS 3.14.3)

> certutil: Problem creating SubjectAltName extension: Certificate extension
> not found.
> certutil: unable to create cert (Certificate extension not found.)

I was able to fix this problem, too.

This section in AddExtensions:
  if (emailAddrs || dnsNames) {
     ...
     rv = AddEmailSubjectAlt(arena, &namelist, emailAddrs);
     rv |= AddDNSSubjectAlt(arena, &namelist, dnsNames);
     ...
  }

must be changed, because both functions end up calling AddSubjectAltNames, into which you have introduced a new failure scenario. In the above sequence of calls, any of emailAddrs or dnsNames could be NULL, which now causes AddSubjectAltNames to fail, instead of being a no-op.

I replaced above section with:

    rv = SECSuccess;
    
    if (emailAddrs) {
	rv |= AddEmailSubjectAlt(arena, &namelist, emailAddrs);
    }

    if (dnsNames) {
	rv |= AddDNSSubjectAlt(arena, &namelist, dnsNames);
    }

This scratch build with patch v4 succeeded and passed the test suite:
https://brewweb.devel.redhat.com/taskinfo?taskID=5697441

Dmitri, could you please test it on a RHEL 6 system?

Bob, does it make sense to clarify, using which command line syntax the enhanced certutil can implement the feature that Dmitri has asked for?

Created attachment 740894 [details]
patch v4 (for NSS 3.15)

I think the original request could also get some clarification.
It would be good to make the list of attributes that cannot currently be created with certutil.

Looking at the link given in the initial comment:
- it apparently asks for flexible subject-alt-names,
  which Bob has implemented with the patch
- it also seems to require an extended-key-usage attribute
  with an arbitrary OID value. I don't know if this is possible yet.

Anything else that I missed?

This is OpenSSL template we use, named reqcfg:

[ req ]
default_bits       = 2048
distinguished_name = req_distinguished_name
attributes         = req_attributes
prompt             = no
output_password    = $PASSWORD

[ req_distinguished_name ]
$SUBJBASE
$CERTNAME

[ req_attributes ]
challengePassword = A challenge password

$SUBJBASE is something like O=EXAMPLE.COM
$CERTNAME is something like CN=ipa.example.com

/usr/bin/openssl req -new -config reqcfg -subj CN=ipa.example.com/O=EXAMPLE.COM -key key_fname -out kdc.req

Okay, let me try to clarify what we're trying to get into certificates.

In RFC4556, a KDC certificate is, ideally, identified by a couple of things.  First there's the id-pkinit-KPKdc EKU value (1.3.6.1.5.2.3.5), and second there's the principal name.  The principal name (marked by the OID 1.3.6.1.5.2.2) is a DER structure, so the patch's current expectation (assuming I'm reading it right) that the value being passed in is a printable string isn't going to work dependably.

Client certificates carry the id-pkinit-KPClientAuth EKU value (1.3.6.1.5.2.3.4), but IPA doesn't issue them yet.

Certificates you'll find on a smart card often have the Microsoft KP SmartCard Logon EKU (1.3.6.1.4.1.311.20.2.2), and format the principal name as a DER UTF8 String marked with OID 1.3.6.1.4.1.311.20.2.3 (Microsoft NT Principal Name), instead of what RFC 4556 specifies.

So while being able to add arbitrary EKU values would be nice, for our purposes we're only interested in PKINIT KDC (1.3.6.1.5.2.3.5), PKINIT Client (1.3.6.1.5.2.3.4), and Microsoft KP SmartCard Logon (1.3.6.1.4.1.311.20.2.2).

Anyway, I installed the scratch build and ran certutil like so:
  certutil -S -d /tmp -n KDC --extSAN other:1.3.6.1.5.2.2\;krbtgt/EXAMPLE.COM -t ,, -s cn=KDC -x -6

The resulting certificate didn't appear to have an subjectAltName extension in it:
-----BEGIN CERTIFICATE-----
MIIBozCCAQygAwIBAgIFAJtwIlswDQYJKoZIhvcNAQEFBQAwDjEMMAoGA1UEAxMD
S0RDMB4XDTEzMDQyOTE0NTUyNVoXDTEzMDcyOTE0NTUyNVowDjEMMAoGA1UEAxMD
S0RDMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCjulMteU/IMcGXaokPMA/M
/kkR2tGthGGpYadsq2Bg+/BkfP3vbYe8AjLF2ENPJumq54HRKlcOT4ndhfhlEIVd
a4p+fpPzkSbWkgnU8+nCG6EllGpYas/LcsRRABiu0SQaYuFR23Eu1ERoIrNqqR0c
OiJMeobMU1w3g6BO4sAkgwIDAQABow0wCzAJBgNVHSUEAjAAMA0GCSqGSIb3DQEB
BQUAA4GBAHLtqGAjL+PJVlHtCFsuLb/gfFf1ljQJzI9CJDwcKDBGt0XWbTxnhkeM
HMtfSeIoGebeDyVyIdyM1Qxwva2QNyiC4LQ90HRxTuMgoBBaO73UMxotE1W8No/l
IEETuWZ8Q59HIwRVYEImM6aeVPhJu9MDBOs6ZwaDKisEdOJTSOCq
-----END CERTIFICATE-----

Hopefully I was just invoking it incorrectly.  Do you have an example of how the --extSAN option is expected to be used?

Rob, Nalin, thank you for the additional details.

There are several issues with the attached patch. I confirm the (apparently untested) patch doesn't yet create an "other" SAN. (There was a typo which lead to an incorrect variable being used, thus it had passed compiling, but resulted in in correct behaviour. Also, the code path for actually adding the extension wasn't reached.)

Still, with the above issues fixed, the result is still incorrect. When dumping a certificate created by the patch, it results in an error message, because of an invalid DER data sequence.

The current patch assumes that the data part of the otherName can be any value, and it simply adds the given string. But in my understanding that's wrong - the data part of otherName still must be a valid encoded DER sequence.

The patch assumes that it's possible to have a general purpose implementation for adding otherName.

I think that's insufficient. It's necessary to know/implement the structure definition of each otherName that we want to implement.

I searched around, and I found an example certificate at
  http://ftp.samba.org/pub/pub/unpacked/lorikeet-heimdal/lib/hx509/data/kdc.crt
which contains:
379  72:         SEQUENCE {
381   3:           OBJECT IDENTIFIER subjectAltName (2 5 29 17)
386  65:           OCTET STRING, encapsulates {
388  63:             SEQUENCE {
390  61:               [0] {
392   6:                 OBJECT IDENTIFIER '1 3 6 1 5 2 2'
400  51:                 [0] {
402  49:                   SEQUENCE {
404  13:                     [0] {
406  11:                       GeneralString 'TEST.H5L.SE'
       :                       }
419  32:                     [1] {
421  30:                       SEQUENCE {
423   3:                         [0] {
425   1:                           INTEGER 1
       :                           }
428  23:                         [1] {
430  21:                           SEQUENCE {
432   6:                             GeneralString 'krbtgt'
440  11:                             GeneralString 'TEST.H5L.SE'
       :                             }
       :                           }
       :                         }
       :                       }
       :                     }
       :                   }
       :                 }
       :               }
       :             }

while our current (fixed) patch produces:
265  51:         SEQUENCE {
267   3:           OBJECT IDENTIFIER subjectAltName (2 5 29 17)
272  44:           OCTET STRING, encapsulates {
274  42:             SEQUENCE {
276  40:               [0] {
278   6:                 OBJECT IDENTIFIER '1 3 6 1 5 2 2'
286  30:                 [0] {
288 114:                   [APPLICATION 11] {
290 116:                     [APPLICATION 2] {
292 116:                       [APPLICATION 7] {
294  69:                         Unknown (Reserved) {
296  65:                           [APPLICATION 24]
       :                   'MPLE.COM...U.%..0...+.......0...*.H'
       :                   '............ &...'
       :                   Error: IA5String contains illegal character(s).
363   4:                           Unknown (Reserved)
       :                             Unrecognised primitive, hex value is: 51 B8 40 7D
Error: Inconsistent object length, 4 bytes difference.
       :                           }


I further searched for the definition of this otherName. From
http://www.h5l.org/manual/HEAD/info/heimdal/Setting-up-PK_002dINIT.html :

   The data part of the OtherName is filled with the following DER 
   encoded ASN.1 structure:

            KRB5PrincipalName ::= SEQUENCE {
              realm [0] Realm,
              principalName [1] PrincipalName
            }

   where Realm and PrincipalName is defined by the Kerberos 
   ASN.1 specification.


from https://cwiki.apache.org/confluence/display/DIRxPMGT/Kerberos+review

       KerberosString = GeneralString = (IA5String)

       Realm = KerberosString


from https://cwiki.apache.org/confluence/display/DIRxPMGT/Kerberos+PrincipalName

       PrincipalName   ::= SEQUENCE {
               name-type       [0] Int32,
               name-string     [1] SEQUENCE OF KerberosString
       }

     name-type 1 = Just the name of the principal as in DCE, or for users


This means, if we want to teach certutil how to encode this extension, we'll need a new option that takes parameters:
- realm (string)
- name-type (number. e.g. 1)
- sequence of name-strings (e.g. principal, realm)


I believe most of the code in this patch is useful in general and should be added, but it isn't solving the issue here. We might consider to remove support for the otherName type from this patch, it seems to me it's not helpful, as we don't have a generic way to dynamically describe the respectively required internal structure.


So, we can either implement a hardcoded new option for certutil that implements the PKINIT/KDC extension described here, or we can make use of a new general mechanism to embed externally created extensions, which I've started to work on and described at https://bugzilla.mozilla.org/show_bug.cgi?id=969822

Created attachment 861478 [details]
python script that uses pyasn1 to encode a raw DER pkinit-subject-alt-name extension

This script creates the extension you're looking for and writes it to a file. This is for demonstration purposes. For simplicity, the input values and output filename are fixed in the script.

The output file is a SUBSET of a certificate. The intention is to use it with certutil's upcoming new parameter --extGeneric

The dump of the output file created by the script is:

  0  87: SEQUENCE {
  2  85:   [0] {
  4   6:     OBJECT IDENTIFIER '1 3 6 1 5 2 2'
 12  75:     [0] {
 14  73:       SEQUENCE {
 16  25:         [0] {
 18  23:           GeneralString 'EXAMPLE.COM'
       :           }
 43  44:         [1] {
 45  42:           SEQUENCE {
 47   3:             [0] {
 49   1:               INTEGER 1
       :               }
 52  35:             [1] {
 54  33:               SEQUENCE {
 56   6:                 GeneralString 'krbtgt'
 64  23:                 GeneralString 'EXAMPLE.COM'
       :                 }
       :               }
       :             }
       :           }
       :         }
       :       }
       :     }
       :   }

Created attachment 861481 [details]
experimental patch 6

This is a work-in-progress patch against NSS, which also contains some unrelated changes, which adds the --extGeneric feature to certutil.

Created attachment 861482 [details]
binary extension file created by the python script

Created attachment 861486 [details]
experimental patch v7

Using the attached binary file, and a certutil build with patch v7, use the following command to create a cert:

certutil -d sql:$PWD -S -n KDC -t ,, -s cn=KDC -x \
  --extGeneric 2.5.29.17:not-critical:$PWD/pkinit.san-extension.der 

Result is a certificate with the following section:

certutil -d sql:$PWD -L -n KDC -r | pp -t certificate
...
    Name: Certificate Subject Alt Name
    Other Name: Sequence {
	[0]: {
	    1b:17:45:58:41:4d:50:4c:45:2e:43:4f:4d:40:45:58:
	    41:4d:50:4c:45:2e:43:4f:4d
	}
	[1]: {
	    Sequence {
		[0]: {
		    1 (0x1)
		}
		[1]: {
		    Sequence {
			1b:06:6b:72:62:74:67:74
			1b:17:45:58:41:4d:50:4c:45:2e:43:4f:4d:40:45:
			58:41:4d:50:4c:45:2e:43:4f:4d
		    }
		}
	    }
	}
    }
	OID: OID.1.3.6.1.5.2.2
...


You can use the new command line option --dump-ext-val to extract the binary contents:

certutil -d sql:$PWD -L -n KDC --dump-ext-val 2.5.29.17 | dumpasn1 -

  0  87: SEQUENCE {
  2  85:   [0] {
  4   6:     OBJECT IDENTIFIER '1 3 6 1 5 2 2'
 12  75:     [0] {
 14  73:       SEQUENCE {
 16  25:         [0] {
 18  23:           GeneralString 'EXAMPLE.COM'
       :           }
 43  44:         [1] {
 45  42:           SEQUENCE {
 47   3:             [0] {
 49   1:               INTEGER 1
       :               }
 52  35:             [1] {
 54  33:               SEQUENCE {
 56   6:                 GeneralString 'krbtgt'
 64  23:                 GeneralString 'EXAMPLE.COM'
       :                 }
       :               }
       :             }
       :           }
       :         }
       :       }
       :     }
       :   }

I think our focus has moved from using certutil to generate certificates for IPA to having Dogtag issue them instead.  Rob, can you confirm my reading of that, as of https://fedorahosted.org/freeipa/ticket/3494 being fixed?

Anyway, looking at the certutil parts of attachment #8394996 posted to Mozilla's #970539, attempting to pass in a principal name using the 'other' subjectAltName type and the '--extSAN' option wouldn't currently work well because the principal name is encoded as a DER sequence, and certutil would appear to still be expecting a a NUL-terminated string.

Eschewing that and using '--extGeneric' to add a subjectAltName extension directly looks like it would allow us to provide Kerberos principal name values in either or both the RFC4556 format and the format Windows uses, so that's useful.

I think we'd still be missing the ability to add either the specific OID values from comment #19 and/or arbitrary ones, unless '--extGeneric' is meant to be used for that, too.

(In reply to Nalin Dahyabhai from comment #31)
> 
> I think we'd still be missing the ability to add either the specific OID
> values from comment #19 and/or arbitrary ones, unless '--extGeneric' is
> meant to be used for that, too.

Yes, it's meant to be used for that, too.
You would be able to add arbitrary ones, using a two step procedure.

You would use another tool, for example the script from attachment 861478 [details], that you can easily manipulate to contain any values you require, to create the binary representation of the extension and save it to a file.

Then you would use --extGeneric with that binary file together with all the other parameters for certutil that you require.

(In reply to Nalin Dahyabhai from comment #31)

Yes, the original reason for this BZ is a non-issue now that we have dropped support for the NSS command-line-based CA in IPA.

The spirit lives on though, to improve the certutil command line to be on-par with openssl.

So, can you please confirm that my description is seen as an enhancement that you will benefit from?

I need this confirmation from anyone to proceed with this work, or I'll stop it.

We don't have the resources to re-implement the full equivalent of the openssl parser, the combined offering here is a reasonable compromise that solves your problem with a reasonable amount of resources.

(In reply to Kai Engert (:kaie) from comment #34)
> So, can you please confirm that my description is seen as an enhancement
> that you will benefit from?

I can't provide such confirmation -- the IPA feature which used certutil to issue certificates for IPA clients has been removed in favor of using Dogtag.

would you like to mark this bug as wontfix, if you don't need it any longer?

Closing it as won't fix because the requester no longer needs the enhancement.

FWIW, we've implemented the proposed solutions anyway.
They just landed upstream, and should be available with NSS 3.16.2