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
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
