[Freeipa-devel] [PATCH] 0097 Add options to write lightweight CA cert or chain to file

Fri Aug 26 08:28:58 UTC 2016

On 19.8.2016 13:11, Fraser Tweedale wrote:
> Bump for review.
>
> On Wed, Aug 17, 2016 at 12:09:39AM +1000, Fraser Tweedale wrote:
>> On Tue, Aug 16, 2016 at 08:10:08AM +0200, Jan Cholasta wrote:
>>> On 16.8.2016 07:24, Fraser Tweedale wrote:
>>>> On Mon, Aug 15, 2016 at 08:19:33AM +0200, Jan Cholasta wrote:
>>>>> On 9.8.2016 16:47, Fraser Tweedale wrote:
>>>>>> On Mon, Aug 08, 2016 at 10:49:27AM +0200, Jan Cholasta wrote:
>>>>>>> On 8.8.2016 09:06, Fraser Tweedale wrote:
>>>>>>>> On Mon, Aug 08, 2016 at 08:54:05AM +0200, Jan Cholasta wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 8.8.2016 06:34, Fraser Tweedale wrote:
>>>>>>>>>> Please review the attached patch with adds --certificate-out and
>>>>>>>>>> --certificate-chain-out options to `ca-show' command.
>>>>>>>>>>
>>>>>>>>>> Note that --certificate-chain-out currently writes a bogus file due
>>>>>>>>>> to a bug in Dogtag that will be fixed in this week's build.
>>>>>>>>>>
>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/6178
>>>>>>>>>
>>>>>>>>> 1) The client-side *-out options should be defined on the client side, not
>>>>>>>>> on the server side.
>>>>>>>>>
>>>>>>>> Will option defined on client side be propagated to, and observable
>>>>>>>> in the ipaserver plugin?  The ipaserver plugin needs to observe that
>>>>>>>> *-out has been requested and executes additional command(s) on that
>>>>>>>> basis.
>>>>>>>
>>>>>>> Is there a reason not to *always* return the certs?
>>>>>>>
>>>>>> We hit Dogtag to retrieve them.
>>>>>
>>>>> I don't think that's an issue in a -show command.
>>>>>
>>>> cert_show is invoked by other commands (cert_find*, cert_show,
>>>> cert_request, cert_status, ca_del) but these all hit Dogtag anyway
>>>> so I suppose that's fine.  I'll return the cert *and* the chain in
>>>> separate attributes, unconditionally.
>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 2) I don't think there should be additional information included in summary
>>>>>>>>> (and it definitely should not be multi-line). I would rather inform the user
>>>>>>>>> via an error message when unable to write the files.
>>>>>>>>>
>>>>>>>> I was just following the pattern of other commands that write certs,
>>>>>>>> profile config, etc.  Apart from consistency with other commands I
>>>>>>>> agree that there is no need to have it.  So I will remove it.
>>>>>>>>
>>>>>>>>> If you think there is an actual value in informing the user about
>>>>>>>>> successfully writing the files, please use ipalib.messages for the job.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 3) IMO a better format for the certificate chain than PKCS#7 would be
>>>>>>>>> concatenated PEM, as that's the most commonly used format in IPA (in
>>>>>>>>> installers, there are no cert chains in API commands ATM).
>>>>>>>>>
>>>>>>>> Sure, but the main use case isn't IPA.  Other apps require PKCS #7
>>>>>>>> or concatenated PEMs, but sometimes they must be concatenated
>>>>>>>> forward, and othertimes backwards.  There is no one size fits all.
>>>>>>>
>>>>>>> True, which is exactly why I think we should at least be self-consistent and
>>>>>>> use concatenated PEM (and multi-value DER over the wire).
>>>>>>>
>>>>>> Dogtag returns a PKCS7 (either DER or PEM, according to HTTP Accept
>>>>>> header).
>>>>>>
>>>>>> If we want list-of-PEMs between server and client we have to convert
>>>>>> on the server.  Do we have a good way of doing this without exec'ing
>>>>>> `openssl pkcs7' on the server?  Is it acceptable to exec 'openssl'
>>>>>> to do the conversion on the server?  python-nss does not have PKCS7
>>>>>> functions and I am not keen on adding a pyasn1 PKCS7 parser just for
>>>>>> the sake of pushing bits as list-of-PEMs.
>>>>>
>>>>> I'm afraid we can't avoid conversion to/from PKCS#7 one way or the other.
>>>>> For example, if we added a call to retrieve external CA chain using certs
>>>>> from cn=certificates,cn=ipa,cn=etc, we would have to convert the result to
>>>>> PKCS#7 if it was our cert chain format of choice.
>>>>>
>>>>> What we can avoid though is executing "openssl pkcs7" to do the conversion -
>>>>> we can use an approach similar to our DNSSEC code and use python-cffi to
>>>>> call libcrypto's PKCS#7 conversion routines instead.
>>>>>
>>>> I had a look at the OpenSSL API for parsing PKCS #7; now I prefer to
>>>> exec `openssl' to do the job :)
>>>>
>>>> I will transmit DER-encoded PKCS #7 object on the wire; we cannot
>>>> used multi-valued DER attribute because order is important.   Client
>>>> will convert to PEMs.
>>>
>>> Well, my point was not to send PKCS#7 over the wire, so that clients
>>> (including 3rd party clients) do not have to convert from PKCS#7 themselves.
>>>
>>> In fact we can use multi-valued DER - whatever you send over the wire from
>>> the server will be received in the exact same order by the client. Even if
>>> it wasn't, you can easily restore the order by matching issuer and subject
>>> names of the certificates.
>>>
>>>>
>>>> Should have new patch on list this afternoon.
>>>>
>>>> Thanks,
>>>> Fraser
>>>>
>>>>>>
>>>>>> FWIW, man pages and code suggest that PKCS #7 is accepted in
>>>>>> installer, etc.
>>>>>
>>>>> True, but that's a relatively new feature (since 4.1) and the installer
>>>>> internally executes "openssl pkcs7" to convert PKCS #7 to list of certs :-)
>>>>>
>>>>>>
>>>>>>>> We can add an option to control the format later, but for now,
>>>>>>>> Dogtag returns a PKCS #7 (PEM or DER) so let's go with that.  Worst
>>>>>>>> case is an admin has to invoke `openssl pkcs7' and concat the certs
>>>>>>>> themselves.
>>>>>>>
>>>>>>> AFAIK none of NSS, OpenSSL or p11-kit can use PKCS#7 cert chains directly,
>>>>>>> so I'm afraid the worst case would happen virtually always.
>>>>>>>
>>>>>> If you're OK with invoking OpenSSL on the client to convert PKCS #7
>>>>>> to list-of-PEMs (similar to what is done in
>>>>>> ipapython.certdb.NSSDatabase) then we can have the client perform
>>>>>> the conversion.
>>>>>
>>>>> See above.
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 4) Over the wire, the certs should be DER-formatted, as that's the most
>>>>>>>>> common wire format in other API commands.
>>>>>>>>>
>>>>>>>> OK.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 5) What is the benefit in having the CA cert and the rest of the chain
>>>>>>>>> separate? For end-entity certs it makes sense to separate the cert from the
>>>>>>>>> CA chain, but for CA certs, you usually want the full chain, no?
>>>>>>>>>
>>>>>>>> If you want to anchor trust directly at a subca (e.g. restrict VPN
>>>>>>>> login to certs issued by VPN sub-CA) then you often just want the
>>>>>>>> cert.  The chain option does subsume it, at cost of more work for
>>>>>>>> administrators with this use case.  I think it makes sense to keep
>>>>>>>> both options.
>>>>>>>
>>>>>>> Does it? From what you described above, you either want just the sub-CA
>>>>>>> cert, or the full chain including the sub-CA cert, in which case it might
>>>>>>> make more sense to have a single --out option and a --chain flag.
>>>>>>>
>>>>>> How about --certificate-out which defaults to single cert, but does
>>>>>> chain (as list-of-PEMs) when --chain flag given.
>>>>>>
>>>>>> Per https://fedorahosted.org/freeipa/ticket/5166 let's not add more
>>>>>> `--out' options.
>>>>>
>>>>> +1
>>>>>
>> Updated patch 0097-2 attached, and new patch 0099 which must be
>> applied first.
>>
>> I have implemented the suggested changes, except for cffi (I execute
>> `openssl pkcs7' instead).

I don't like it, but OK. Another alternative would be to use pyasn1.

>>
>> There are two new output attributes on the wire, 'certificate'
>> (single-value DER X.509), and 'certificate_chain' (ordered
>> multi-value DER X.509).  They are always returned.  The first cert
>> in the chain is always the same as 'certificate'; obviously this is
>> redunant but I have left it this way because I think usage is
>> clearer.

I don't have a strong feeling about this one way or the other, but the 
same scheme should be used for cert-show in the future. Does it make 
sense to do it this way for cert-show?

I'm not sure about always returning the chain in cert-show. Now that we 
have a --chain flag rather than two out options, maybe we should go back 
to returning the chain only if --chain is specified. What do you think?

Patch 0099:

1) Please fix this:

$ git show -U0 | pep8 --diff
./ipalib/x509.py:59:80: E501 line too long (93 > 79 characters)

Patch 0097:

1) `certificate` and `certificate_chain` are actually attributes of the 
ca object, so they should be defined in ca.takes_params rather than 
ca_show.has_output_params.

2) Please fix these:

$ git show -U0 | pep8 --diff
./ipaclient/plugins/ca.py:21:9: E124 closing bracket does not match 
visual indentation
./ipaclient/plugins/ca.py:23:13: E128 continuation line under-indented 
for visual indent
./ipaclient/plugins/ca.py:24:13: E128 continuation line under-indented 
for visual indent
./ipaclient/plugins/ca.py:25:13: E128 continuation line under-indented 
for visual indent
./ipaclient/plugins/ca.py:26:9: E124 closing bracket does not match 
visual indentation
./ipaclient/plugins/ca.py:38:13: E731 do not assign a lambda expression, 
use a def

Honza

-- 
Jan Cholasta