On 19.8.2016 13:11, Fraser Tweedale wrote:
Bump for review.

On Mon, Aug 15, 2016 at 05:15:16PM +1000, Fraser Tweedale wrote:
Thanks for reviews.  Rebased and updated patches attached (and one
new patch).  No substantive changes to 92..94.  Patch order is:

92-2, 93-2, 94-2, 98, 90-3

Other comments inline.

Thanks,
Fraser

On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote:
Patch 0092: ACK

Patch 0093: ACK

Patch 0094: ACK

Please fix this PEP8 issue before pushing:

./ipaserver/plugins/cert.py:597:17: W503 line break before binary operator


Patch 0098: ACK


Patch 0090:

1) Generic otherNames (san_other) do not work correctly. The OID is not
included in the value and names with complex type other than
KerberosPrincipal are not parsed correctly. The value should include the OID
and DER blob of the name.

Updated to use "OID:b64(DER)" as the attribute value.

OK.


2) With --all, san_other should be included in the result for all
otherNames, even the known ones, to provide (limited) forward compatibility.

Done; when --all given, known otherName kinds are included in
'san_other' attribute in addition to their own attribute.

OK.


3) Do we have to support *all* the name types? I mean we could, for the sake
of completeness, but it might be easier to just keep the few ones we
actually care about (email, DNS name, principal name, UPN and directory name
in your patch 0095).

Yeah, why not support them all?  See also Petr's comments in other
branch of thread.

Works for me, but see Lukáš's reply, I think he has a point. Maybe we can make a compromise and show only supported name types by default and everything with --all?


4)

+            obj.setdefault(attr_name, []).append(unicode(name))

The value should not (always) be unicode, but of the type declared by the
param (unicode or ipapython.kerberos.Principal or
ipapython.dnsutil.DNSName).

I now pass the value to the constructor of whatever type the
parameter uses:

        attr_value = self.params[attr_name].type(name_formatted)
        obj.setdefault(attr_name, []).append(attr_value)

OK.


5) san_directoryname should be a DNParam rather than Str.


6) Could we use "Subject <name type>" instead of "Subject Alternative Name (<name type>)" for labels? Or something else which is shorter and has the name type more "visible" than the current form.


7) The patch needs a rebase.


--
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

Reply via email to