Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve pretty-printer for DNs (RFC 4514 and OSF). #119

Merged
merged 2 commits into from
Oct 9, 2019

Conversation

paurkedal
Copy link
Contributor

@paurkedal paurkedal commented Oct 7, 2019

This is my suggestion for an improved DN pretty-printing, with some reservations:

  1. I still cannot find any specifications for the OSF DN format, so that was designed in analogy with the RFC format and what I know from working with such DNs. The uncertain part is escaping rules. I consequently switched the default pp to the RFC format.

  2. It should be noted that openssl uses something which looks like RFC 4514 but prints the RDNs in opposite order, e.g. openssl -issuer of by own cert is C = NL, ST = Noord-Holland, L = Amsterdam, O = TERENA, CN = TERENA eScience Personal CA 3. I think this is a source of ambiguity, since its hard for a parser to determine which order is used, apart from heuristics on certain attribute-types. The OpenSSL order would have been better, but section 2.2 states the opposite. I found a stackoverflow entry about it.

  3. Since line breaks are allowed before RDNs, I inserted braking hints, but opted to wrap the default printer pp in a hbox to avoid surprises, e.g. when creating a string to pass to a configuration file. When referring to a DNs within a text it is better to skip the box (and add < > quotes around it).

  4. Section 2.4 says that the hex-encoding of the attribute value must be used if the attribute type is in dotted-decimal form. This makes sense, since one would usually use the dotted-decimal form for unknown attribute types, where one also cannot know that the value is a DictionaryString.

Given point 2, maybe we should be less insistent and let make_pp flexible enough to support the OpenSSL format, and maybe switch back pp to the OSF format, which is unambiguous. A parser can easily distinguish OSF from one of the other formats due to the / prefix.

tests/regression.ml Outdated Show resolved Hide resolved
@hannesm
Copy link
Member

hannesm commented Oct 8, 2019

thanks for your contribution

we should be less insistent and let make_pp flexible enough to support the OpenSSL format

yes, that would be good!

switch back pp to the OSF format, which is unambiguous

that'd be fine with me as well

@paurkedal
Copy link
Contributor Author

I sketched out the interface and docs of a new version. I'll take further any suggestions and implement it in the evening:

  (** [make_pp ()] creates a pretty-printer customized according to the optional
      arguments.

      @param format
        Determines order of relative distinguished names (RDNs), the default
        separators, and escaping of attribute values:
        - [`RFC4514] to produce the
          {{:https://tools.ietf.org/html/rfc4514}RFC4514} format, where RDNs are
          written in reverse order and the default RDN separator is a comma
          followed by a cut as RDN separator.
        - [`OpenSSL] produces the same format as [`RFC4514], except than RDNs
          are not reversed.
        - [`OSF] emits RDNs prefixed by slashes and separated by cuts in
          non-reversed order.  This format is designed by analogy to RFC4514,
          and may not be fully compliant to the OSF specifications, if it
          exists.
      @param rdn_sep
        The separator between RDNs.  For RFC4514 compliance either a comma or a
        semicolon must be used, with optional whitespace on either side.  If you
        set this for the OSF format, the slash prefix will be omitted, and must
        be re-added e.g. with [Fmt.(any "/" ++ pp)].
      @param ava_sep
        The separator between attribute value assertions (AVAs).  For RFC4514
        compliance, this must be a plus sign surrounded by optional whitespace.
        A line break is only allowed after the plus sign.
      @param ava_equal
        The separator between the attribute type and value.  For RFC4514
        compliance, this must be an equal sign surrounded by optional
        non-breaking space.

      The pretty-printer can be wrapped in a box to control line breaking and
      set it apart, otherwise the RDN componets will flow with the surrounding
      text. *)
  val make_pp :
    ?format: [`RFC4514 | `OpenSSL | `OSF] ->
    ?rdn_sep: unit Fmt.t ->
    ?ava_sep: unit Fmt.t ->
    ?ava_equal: unit Fmt.t ->
    unit -> t Fmt.t

@hannesm
Copy link
Member

hannesm commented Oct 8, 2019

this looks good (though has a rather large matrix of choices). the default pp will then be ~format:`OSF ~rdn_sep:(unit "/") ~ava_sep:(unit "+") ~ava_equal:(unit "=")? would it make sense to instead of ava_sep and ava_equal only have a surround_with_whitespaces? (i.e. I'd expect that either you want WS everywhere or nowhere - but I may be wrong)

@paurkedal
Copy link
Contributor Author

I just pushed a new version with slightly different interface from my last post.

@hannesm
Copy link
Member

hannesm commented Oct 8, 2019

@paurkedal cool, I'll hopefully have a look at it this evening

@paurkedal
Copy link
Contributor Author

paurkedal commented Oct 8, 2019

About surround_with_whitespace, I considered doing that from the start to prevent generating invalid formats. I am a bit undecided what is best, as there is a bit more choices one might want to make like adding spaces only for the outer parts but not around the equality sign, or adding highlighting. Note also breaking hints are asymmetric (no breaks before comma or plus sign). The current proposal went in the other direction and added a fuller choice-matrix.

@paurkedal
Copy link
Contributor Author

If we want to limit configuration of the separators to the spacing, I think it would suffice to have three choices: 1) add no extra space, 2) add space after comma and around plus, 3) also add space around the equality sign. And in all cases, add the same allowed cuts.

@hannesm
Copy link
Member

hannesm commented Oct 8, 2019

I'd be in favour of avoiding invalid formats (i.e. not providing the flexibility to specify your own separator), and agree with the three options you pointed out above.

@paurkedal
Copy link
Contributor Author

Yes, I think I agree. I'll update with something like

val make_pp :
  ?format: [`RFC4514 | `OpenSSL | `OSF] ->
  ?spacing: [`Tight | `Medium | `Loose] ->
  unit -> t Fmt.t

tomorrow.

@paurkedal paurkedal changed the title WiP: Improve pretty-printer for DNs (RFC 4514 and OSF). Improve pretty-printer for DNs (RFC 4514 and OSF). Oct 9, 2019
@paurkedal
Copy link
Contributor Author

How about this revision?

@hannesm
Copy link
Member

hannesm commented Oct 9, 2019

this looks fine to me, thanks!

@paurkedal
Copy link
Contributor Author

Great, but I see a docstring error, committing a fix now.

@hannesm hannesm merged commit 430b119 into mirleft:master Oct 9, 2019
@paurkedal paurkedal deleted the improve-dn-pp branch October 9, 2019 19:18
hannesm added a commit to hannesm/opam-repository that referenced this pull request Oct 10, 2019
CHANGES:

* export X509.Distinguished_name.common_name : t -> string option, which
  extracts the common name of a distinguished name
* Distinguished_name.t is now a Relative_distinguished_name.t list, a
  Relative_distinguished_name is a Set.S with element type attribute, a variant.
  It used to be an attribute (expressed as GADT) Gmap.t, but this representation
  did not conform to RFC 5280, reported by @paurkedal (mirleft/ocaml-x509#117, fixed by mirleft/ocaml-x509#118)
* Now using Set.find_first_opt, which bumps lower OCaml bound to 4.05.0
* Improved pretty-printing for DNs including RFC 4514 conformance (@paurkedal, mirleft/ocaml-x509#119).
* Extension.pp now outputs extension key and its value (mirleft/ocaml-x509#120)
* rename Distinguished_name.SP constructor (stateOrProvince) to ST, as widely used (mirleft/ocaml-x509#121)
* support Street and UID in Distinguished_name to satisfy RFC 4514 demands (mirleft/ocaml-x509#121)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants