[Pki-devel] [PATCH] PKCS10Client enhancement

[Andrew Wnuk]

On 08/15/2013 09:16 PM, Ade Lee wrote:
> On Thu, 2013-08-15 at 16:29 -0700, Andrew Wnuk wrote:
>> On 08/15/2013 09:47 AM, Ade Lee wrote:
>>> Couple of questions:
>>>
>>> 1. I am assuming that ":" is not a valid  character in each of the
>>> subject name fields, right?
>> ":" is not on the list of special characters required to be escaped, so
>> it is a valid character as names are validated. You can see name
>> validation in the code.
>>
>> Keep in mind that this tool modification is added only to test one very
>> unique incompatibility of some cryptographic libraries which are not
>> processing subject names in their proper canonical form, which can only
>> be seen in one corner case of cross signing.
>>
>> There are few options to introduce encoding types:
>> 1. Introduce encoding types as a separate set parameters.
>> 2. Introduce distinguished name preprocessing, collect encoding types
>> from preprocessor and distribute them to component name encoders.
>> 3. Use above syntax only if enabled by flag
>> 4. Use above syntax only if prefix includes allowed encoding typesfor
>> specific name component
>>
>> Option (1) seems inconvenient and error prone
>> Option (2) seems a bit expensive for its purpose
>> Option (3) seems reasonable but new enabling flag seems a bit inconvenient
>> I believe I selected option (4) but attached the wrong file.
>> I selected option (4) base on expense and the purpose of this tool.
> I am OK with using option (4) given the limited scope of this
> tool/usage.  However, given that ":" is a valid character, the code as
> you currently have it could break on strings that contain a ":".
>
> For example, "cn=aaaa:bbb, ou=ccc, o=dddd" will be encoded as "ou=ccc,
> o=dddd".  Why?  Because the cn=aaaa:bbb will be parsed as one that is
> supposed to contain an encoding.  When the supposed encoding ("aaaa")
> does not match what you expect, it is silently ignored and dropped.
>
> Right?  Or am I missing something?
I am not quite sure what are you confused about but you either provide 
correct prefix which specifies encoding or entire 'AttributeValue' is 
just a value.
After short discussion with Nathan, we decided to combine options (3) 
and (4) to provide ability to include encoding type prefixes as a value 
for 'AttributeValue'.
>   
>>> 2. Is the format "cn=UTF8String:aa,ou=BMPString:bb,o=cc" a standard way
>>> of specifying this type of thing?  If not, is there a standard way of
>>> specifying subject names with multiple encodings? (and why would anyone
>>> do that?)
>> There is no standard way to include encoding for attribute values in
>> distinguished names.
>> The main reason here is that distinguished names are defined by LDAP
>> standards providing strict encoding rules and strict name processing rules.
>> LDAP is not exposed to our case of cryptographic libraries incorrectly
>> processing subject names.
>>> 3. Right now, if someone puts in an incorrect encoding, the element will
>>> silently fail to be added to the subject name.  Is this what we want?
>>> Do we rather want to throw an exception and notify the user?
>> This will go away with option 4.
>>> 4. Similarly, in addNameElement, we catch the exception and just print
>>> out an error message.  Do we want to rather bubble up the exception and
>>> abort?
>> I think that providing error message is more appropriate here.
> OK
>
>>> 5. Does it make sense to add logic to addNameElement() to default to
>>> PrintableString in the case that n=0, and then just eliminate the calls
>>> to the old functions?
>>>
>>> For example, instead of :
>>>
>>>                  if (split[0].equals("O")) {
>>>                       if (n > 0) {
>>>                           ret = addNameElement (ret, Name.organizationName, n, split[1]);
>>>                       } else {
>>>                           ret.addOrganizationName(split[1]);
>>>                       }
>>>                       //                System.out.println("O found : " + split[1]);
>>>                       continue;
>>>                  }
>>>
>>> have:
>>>                  if (split[0].equals("O")) {
>>>                       ret = addNameElement (ret, Name.organizationName, n, split[1]);
>>>                       // System.out.println("O found : " + split[1]);
>>>                       continue;
>>>                  }
>>>
>>> and remove the function addOrganizationName() etc.  In fact, it probably
>>> make sense to just compute n in the addNameElement() function, rather
>>> than passing it.
>> All "name" functions are provided by JSS, which includes OID to name
>> mapping and default encoding
>> To do the above, you would have to duplicate OID to name mapping and
>> default encoding selection outside JSS, which is an error prone
>> duplication of functionality.
>>
> Well, you're not really duplicating anything in that you are passing the
> OID mapping provided by JSS, and the default is provided by the RFC,
> right?
Currently defaults are embedded in JSS and they are not easily 
accessible and again adding second set of defaults in PKCS10Client tool 
is error prone.
>
> Still, you'll need to change your code to handle the case I mention
> above.  In pseudo-code, I guess you could do something like this:
>
> valid_encodings = {arraylist of encodings}
> encoding = substring(value)
>
> if (split[0].equals("O")) {
>      if (encoding in valid_encodings) {
>          ret = addNameElement (ret, Name.organizationName, encoding, value);
>      } else {
>          ret.addOrganizationName(value)
>      }
> }
>   
That was already done in the correct patch. Now I will add option (3) as 
mentioned above.
>>> 6.  Please add some comments above the addNameElement() function to
>>> describe what is going on. (maybe referencing the ticket number).
>> Sure.
>>> 7.  In the diff at least, it looks like the System.out.println()
>>> statements are a little offset.  I know they are not in your code
>>> changes, but please fix them while we are there.
>> I thought those lines are purposely formatted this way, since all files
>> were reformatted before.
> The reformatting was done over all the code - its very mostly right, but
> there will be some bad cases every so often, particularly in commented
> out code.  We should just fix them as we find them.
>
>>> Thanks,
>>> Ade
>>>    
>>> On Wed, 2013-08-14 at 17:08 -0700, Andrew Wnuk wrote:
>>>> This patch provides enhancement to PKCS10Client allowing to control
>>>> encoding for components of the subject name.
>>>>
>>>> Ticket #677
>>>>
>>>> More details included in comment #2 of ticket #677.
>>>> Testing procedure included in comment #4 of ticket #677.
>>>>
>>>>
>>>> _______________________________________________
>>>> Pki-devel mailing list
>>>> Pki-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/pki-devel
>