Closed JeremyRand closed 7 years ago
Consider using a different item name, e.g. "tlsd", rather than reusing the "tls" item name.
I can understand the reasoning for this, and I think doing so might be cleaner. Unfortunately, doing this will confuse implementations that use "import" in ways that can affect security. I would be more inclined to make this change if https://github.com/ifa-wg/proposals/issues/23 is adopted simultaneously.
Consider using unpadded base64 encoding for the public key and signature.
It's worth considering that while the Golang standard library supports unpadded base64, the Python standard library does not. I also think this may qualify as an attempt to optimize fully, which would be outside of the design goals of this proposal. Indeed, CBOR encoding would achieve much better compression and make the padding issue irrelevant -- so I think that if/when we come to the point that we're worrying about a couple bytes of padding, we should be looking at CBOR. Also, it looks like unpadded base64 is designed for fixed-length data. This spec allows the user to choose which curve to use, so making assumptions about the length of the public key or signature seems like it may break some things.
Also, it looks like unpadded base64 is designed for fixed-length data. This spec allows the user to choose which curve to use, so making assumptions about the length of the public key or signature seems like it may break some things.
Retracting this comment, as @hlandau has kindly informed me via IRC that it's not an issue in our case, since the length of the base64-encoded string is known (since it's derived from JSON).
My other concerns about standard library support and fully optimizing too early are unaffected.
FWIW, if we do want to do further optimization of the size without CBOR, here are 2 possible improvements:
cat public-key-uncompressed.pem | openssl ec -pubin -conv_form compressed -param_enc named_curve
will result in the compressed PEM-encoded pubkey being printed to stdout. I don't know yet whether doing this will cause trouble elsewhere in the Golang codebase, nor do I know whether mainstream web browsers will accept compressed EC pubkeys. This is orthogonal to CBOR.flush
as true, but Python might be able to utilize foldspaces=True
, and Python might need pad=True
(in which case flush
doesn't matter). This is obsoleted by CBOR, but is presumably a lot less disruptive than CBOR is, so could be deployed sooner.I really don't want to overoptimize here, but these changes do seem to be relatively doable. (1) does mean that an OpenSSL command-line binary must be present on the system for at least some operations. (Definitely includes dehydrating a cert; I'm not certain about rehydrating a cert.)
I would lean toward getting something released, and then working on these optimizations (along with other scalability things like evaluating SegVal).
@hlandau any opinions on https://github.com/ifa-wg/proposals/pull/24#issuecomment-226916784 ? Would this be a good excuse to adopt categorization? It would probably be cleaner than using 2 different syntaxes for "tls" items.
ACK
Other future improvement: notice this part of the cert:
X509v3 Extended Key Usage:
TLS Web Server Authentication
Note that this extension doesn't have the "critical" flag. Ryan pointed this out to me while I was doing the CCN hackathon. Neither Ryan nor I can think of any plausible attack scenario that this enables; it would allow someone to misuse a Namecoin TLS server cert for some other purposes [1], but that seems like a pretty lame attack. Nevertheless, fixing it would be a good idea from a theoretical standpoint. Go's standard x509 library doesn't seem to offer an easy way to control this, which is why it wasn't fixed from the beginning.
Again, I do not think this is a blocker bug, but it is worth fixing in the future.
[1] Some other relevant usages in the x509 spec include:
Okay, so upon further thought, I think there are benefits from an implementation being able to reliably predict which items are going to produce a TLSA record, even if the implementation is unable to parse some of those items. These benefits are not completely achieved by the categorization proposal (although categorization helps in some use cases).
One example of this is browser extensions along the lines of HTTPS-Finder, which give users the option to automatically create HTTPS-Everywhere rules. I can imagine some users who would want to create an HTTPS-Everywhere rule for websites that have a TLSA record (to avoid TLS stripping attacks), and some of those users may want to create that rule for sites with unparseable tls
items (and therefore have the connection fail without leaking info). There may be other examples where this helps.
Interestingly, there's already a precedent for this in the Namecoin d/
spec. The i2p
item used to offer 3 syntax options, which were choosable via a dict. We later removed 2 of the syntax options, but I don't think there was ever any suggestion that those syntax options should be separate item types.
So, I'd lean toward rules something like this:
tls
JSON field contains an array of items (just like before).tls
item of the form {"dane":FOO}
, where FOO
contains an array, is interpreted such that FOO
is parsed according to the previously existing rules in the d/
spec for parsing tls
items.tls
item of the form {"d8":FOO}
, where FOO
contains an array, is interpreted such that FOO
is parsed as a dehydrated certificate. (We would need to add a version
field somewhere, not sure if it should be part of the FOO
array or whether it should be part of d8
).tls
item dict (e.g. {"dane":FOO,"d8":BAR}
), implementations must pick a single key that they understand (if one exists) and use it [1].tls
item that consists of an array FOO
is considered to be shorthand for a tls
item of the form {"dane":FOO}
.@hlandau , thoughts on this?
[1] I can't think of any reasons why this would be likely to occur, but it seems like a good idea to specify what behavior occurs in this case.
Changes that @ryancdotorg , @hlandau , and I agreed on via IRC:
@hlandau I've updated this PR; it should now match the code in https://github.com/namecoin/ncdns/pull/16 . Can you review this?
ACK.
@hlandau has made 2 suggestions via IRC.