Here's a COPR if someone wants to just test the functionality:
https://copr.fedorainfracloud.org/coprs/ftweedal/certmonger-v2template

Did you mean to include this?

nit: should you check for NULL?

By returning just TRUE/FALSE here (via NULL) users could have a hard time figuring out where their formatting is bad.

Is this a use-after-free? result is the return value of cm_certext_build_certificate_template() which is created via return SECITEM_ArenaDupItem(arena, &encoded). The arena is freed before result is compared.

There should probably also be tests for each of the validation cases as well as a passing case.

whups, no :)

nope, we're only comparing that the pointer was not-null. The memory pointed to is freed, but we don't need to read it - we only need to know that it was not null (i.e. successfully parsed, therefore input is valid)

I think the additional complexity to get an explanation to the user is not worth it. It's an esoteric feature, and I think it's documented well enough. The immediate use case is for IPA which does its own checking (and if the input is invalid, explaining what's wrong).

Yes I should :)

But this is a certmonger feature and anyone else can do things, get a failure and have no idea what is wrong. Also, documented where?

But this is a certmonger feature and anyone else can do things, get a failure and have no idea what is wrong. Also, documented where?

The expected format is documented in man pages.

I also updated api.txt to describe the expected format.

@rcritten any further comments? How badly do you want a specific error string explaining problems in the template specifier string input? :)

It's fine. We can address it if it comes up. I needed to do some more functional testing. I tested with:

# ipa-getcert request -f /etc/pki/tls/certs/test.pem -k /etc/pki/tls/private/test.pem --ms-template-spec=1.3.6.1.4.1.311.21.8.8950086.10656446.2706058.12775672.480128.147.7130143.4405632:1 -K test/`hostname`

The CSR looks reasonably ok (I didn't bother to decode the value) via openssl req:

    Requested Extensions:
        X509v3 Subject Alternative Name: 
            othername:<unsupported>, othername:<unsupported>
        X509v3 Basic Constraints: critical
            CA:FALSE
        X509v3 Subject Key Identifier: 
            F5:82:2E:CB:45:D5:B1:E3:F1:0B:67:4A:C6:59:10:93:A6:DA:A0:77
        1.3.6.1.4.1.311.21.7: 
            0+.&+.....7.....F...>...

It its a bit difficult to understand where these OIDs come from but given it's MS I'll take it for granted that those admins will know what to pass.

So ACK.

On Wed, Aug 30, 2017 at 05:31:52PM +0000, Rob Crittenden wrote:

rcritten commented on the pull-request: Implement support for MS V2 template extension that you are following:
``
It's fine. We can address it if it comes up. I needed to do some more functional testing. I tested with:

# ipa-getcert request -f /etc/pki/tls/certs/test.pem -k /etc/pki/tls/private/test.pem --ms-template-spec=1.3.6.1.4.1.311.21.8.8950086.10656446.2706058.12775672.480128.147.7130143.4405632:1 -K test/`hostname`

The CSR looks reasonably ok (I didn't bother to decode the value) via openssl req:

    Requested Extensions:
        X509v3 Subject Alternative Name: 
            othername:<unsupported>, othername:<unsupported>
        X509v3 Basic Constraints: critical
            CA:FALSE
        X509v3 Subject Key Identifier: 
            F5:82:2E:CB:45:D5:B1:E3:F1:0B:67:4A:C6:59:10:93:A6:DA:A0:77
        1.3.6.1.4.1.311.21.7: 
            0+.&+.....7.....F...>...

It its a bit difficult to understand where these OIDs come from but given it's MS I'll take it for granted that those admins will know what to pass.

So ACK.
``
The OIDs are identifiers of certificate templates in an AD-CS
installation.

When submitting a CSR to AD-CS, the template specifier (either the
V1 string based one, or the V2 OID-based one) get read, and this
selects the template (profile) to use for issuing the cert.

It's actually an error if you try to submit a CSR that does have one
of these extensions!