Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11184027:
net: add DANE support for DNSSEC stapled certificates. (Closed)
|
Created:
8 years, 2 months ago by agl Modified:
4 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionnet: add DANE support for DNSSEC stapled certificates.
Currently we support a form of CAA record for DNSSEC stapled certificates. Now
that RFC 6698 has been published, we want to change it to use that.
This CL adds support for DANE records in stapled certificates. After this has
reached the stable channel, the old CAA support can be removed.
BUG=none
TEST=Check that https://spki.dane.imperialviolet.org loads without errors.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167227
Patch Set 1 #Patch Set 2 : ... #
Total comments: 20
Patch Set 3 : ... #Patch Set 4 : Syncing with trunk in order to land. #Patch Set 5 : Syncing with trunk in order to land. #
Messages
Total messages: 18 (0 generated)
P.s. if people feel that we should just dump this code, I'm OK with that too. I believe that it has had minimal uptake. I'll have CT code coming at some point soon and dumping this would help offset the complexity of that at least. On Oct 17, 2012 4:06 PM, <agl@chromium.org> wrote: > Reviewers: Ryan Sleevi, wtc, > > Description: > net: add DANE support for DNSSEC stapled certificates. > > Currently we support a form of CAA record for DNSSEC stapled certificates. > Now > that RFC 6698 has been published, we want to change it to use that. > > This CL adds support for DANE records in stapled certificates. After this > has > reached the stable channel, the old CAA support can be removed. > > BUG=none > > > Please review this at https://codereview.chromium.**org/11184027/<https://codereview.chromium.org/1... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M net/base/dns_util.h > M net/base/dnssec_chain_**verifier.h > M net/base/dnssec_chain_**verifier.cc > M net/socket/ssl_client_socket_**nss.cc > > >
Sorry, forgot to publish comments. Presuming that the DNSSEC verification code is correct, the implementation looks correct to me. Mostly, there were a bunch of nits. I support dropping the old CAA code. I'm ambivalent about DANE (as evidenced by my comments in CA/B Forum). I don't think we'll get good results, even using our new DNS resolver code, for DNSSEC, so having this code allows us to point to an actual deployable solution for DANE. The only concern, and as expressed in the CA/B Forum, is how we handle the UI treatment of these. This is already a concern with the CAA code though. Perhaps this is something we should try and talk about at the next IETF, as a proper extension to X.509v3? I think it's a useful method to smuggle the DNSSEC records through. https://codereview.chromium.org/11184027/diff/10001/net/base/dnssec_chain_ver... File net/base/dnssec_chain_verifier.cc (right): https://codereview.chromium.org/11184027/diff/10001/net/base/dnssec_chain_ver... net/base/dnssec_chain_verifier.cc:969: } style nit: indent 'case' to two spaces, inner blocks to four spaces, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_... same below https://codereview.chromium.org/11184027/diff/10001/net/base/dnssec_chain_ver... File net/base/dnssec_chain_verifier.h (right): https://codereview.chromium.org/11184027/diff/10001/net/base/dnssec_chain_ver... net/base/dnssec_chain_verifier.h:169: // usage other than "domain-issued certificate". See nit: mention the specific value (usage 3), since the actual reference to "domain-issued certificate" doesn't appear until you've read all the inner usages text. https://codereview.chromium.org/11184027/diff/10001/net/base/dnssec_chain_ver... net/base/dnssec_chain_verifier.h:171: static void Parse(const std::vector<base::StringPiece>& rrdatas, DESIGN: Usage types 0 - 2 are used to establish pinning data. While it's not necessary to support it yet, it will be important to think about how this structure may need to change to accomodate it, before it's too widely used. https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:335: DnsTLSARecord::Parse(rrdatas, &matches); Should a certificate with invalid record types be accepted? I can see the argument either way, but given that it's security relevant, I'm inclined to suspect the answer should be "As little junk as possible should be accepted". Thoughts? https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:338: i = matches.begin(); i != matches.end(); i++) { style nit: pre-increment http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc... https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:349: } style nit: indentation of cases https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:351: uint8 calculated_hash[SHA512_LENGTH]; // SHA512 is the largest. nit: HASH_LENGTH_MAX (comes from hasht.h, the same place as HASHContext) https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:357: static_cast<HASH_HashType>(i->algorithm), DESIGN: You seem to have tightly coupled the HASH_HashType enum to your int constant. This seems problematic, in that it's easy for the Chromium side to add a new value that is not valid. Perhaps a comment to the appropriate headers explaining this coupling would be suitable. https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:361: DCHECK(rv == SECSuccess); nit: DCHECK_EQ(SECSuccess, rv) https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:433: char port_label_len = 1 + port_str.size(); nit: I would expect a warning here from at least one of our compilers (converting size_t to char - which, I agree, is safe for the constraints of port_str) https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:437: dns_hostname; world's tiniest nit: StringPrintf / StringAppend, per the trend internally to avoid temporary strings?
Review comments on patch set 2: I took a very quick look. This CL seems fine to me. I'll try to take another look tomorrow. If rsleevi is fine with the CL, please feel free to commit it without my review.
On 2012/10/18 23:17:08, agl wrote: > P.s. if people feel that we should just dump this code, I'm OK with that > too. I believe that it has had minimal uptake. This would be fine by me, too. At the NSS face-to-face summit in August, I got the impression that none of the participants was excited about DANE.
I plan on landing this so that it's in the code control history and then removing the whole lot in a subsequent change. But not until we've branched for M24. https://codereview.chromium.org/11184027/diff/10001/net/base/dnssec_chain_ver... File net/base/dnssec_chain_verifier.cc (right): https://codereview.chromium.org/11184027/diff/10001/net/base/dnssec_chain_ver... net/base/dnssec_chain_verifier.cc:969: } On 2012/10/18 23:21:13, Ryan Sleevi wrote: > style nit: indent 'case' to two spaces, inner blocks to four spaces, > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_... > > same below Done. https://codereview.chromium.org/11184027/diff/10001/net/base/dnssec_chain_ver... File net/base/dnssec_chain_verifier.h (right): https://codereview.chromium.org/11184027/diff/10001/net/base/dnssec_chain_ver... net/base/dnssec_chain_verifier.h:169: // usage other than "domain-issued certificate". See On 2012/10/18 23:21:13, Ryan Sleevi wrote: > nit: mention the specific value (usage 3), since the actual reference to > "domain-issued certificate" doesn't appear until you've read all the inner > usages text. Done. https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:335: DnsTLSARecord::Parse(rrdatas, &matches); On 2012/10/18 23:21:13, Ryan Sleevi wrote: > Should a certificate with invalid record types be accepted? I can see the > argument either way, but given that it's security relevant, I'm inclined to > suspect the answer should be "As little junk as possible should be accepted". > Thoughts? Invalid record types are future record types that we just haven't implemented yet, right? Since the record types are a registry it seems that we need to ignore values that we don't know in the same way that we ignore TLS extensions that we don't know. https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:338: i = matches.begin(); i != matches.end(); i++) { On 2012/10/18 23:21:13, Ryan Sleevi wrote: > style nit: pre-increment > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc... Done. https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:349: } On 2012/10/18 23:21:13, Ryan Sleevi wrote: > style nit: indentation of cases Done. https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:351: uint8 calculated_hash[SHA512_LENGTH]; // SHA512 is the largest. On 2012/10/18 23:21:13, Ryan Sleevi wrote: > nit: HASH_LENGTH_MAX (comes from hasht.h, the same place as HASHContext) Done. https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:357: static_cast<HASH_HashType>(i->algorithm), On 2012/10/18 23:21:13, Ryan Sleevi wrote: > DESIGN: You seem to have tightly coupled the HASH_HashType enum to your int > constant. This seems problematic, in that it's easy for the Chromium side to add > a new value that is not valid. The alternative would be Yet Another Hash Type enum, no? The worry is that Chrome adds HASH_SHA3, or some such, which isn't implemented in the system NSS? Since that could happen, I've changed the check of rv to skip records where we fail to hash the input. https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:433: char port_label_len = 1 + port_str.size(); On 2012/10/18 23:21:13, Ryan Sleevi wrote: > nit: I would expect a warning here from at least one of our compilers > (converting size_t to char - which, I agree, is safe for the constraints of > port_str) Added static_cast. https://codereview.chromium.org/11184027/diff/10001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:437: dns_hostname; On 2012/10/18 23:21:13, Ryan Sleevi wrote: > world's tiniest nit: StringPrintf / StringAppend, per the trend internally to > avoid temporary strings? While I agree in general, since |dns_hostname| has an embedded NUL, I think StringPrintf and converting to char* is asking for trouble.
LGTM. I think my only concern is the CAA fallback code, but given that you're going to gut CAA immediately, I think it's fine.
agl: I don't have time to review this CL today. You can commit it with rsleevi's LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/agl@chromium.org/11184027/25001
Change committed as 167227
Message was sent while issue was closed.
On 2012/11/12 19:56:55, I haz the power (commit-bot) wrote: > Change committed as 167227 Hello. The change was been committed 1.5 years ago, but DANE still doesn't work in recent chromium. Why? :(
Message was sent while issue was closed.
On 2014/04/21 12:09:17, xaionaro wrote: > On 2012/11/12 19:56:55, I haz the power (commit-bot) wrote: > > Change committed as 167227 > > Hello. The change was been committed 1.5 years ago, but DANE still doesn't work > in recent chromium. Why? :( This change was only landed to save it in the code history. All DNSSEC stapling support was removed from Chrome.
Message was sent while issue was closed.
On 2014/04/21 17:32:16, agl wrote: > On 2014/04/21 12:09:17, xaionaro wrote: > > On 2012/11/12 19:56:55, I haz the power (commit-bot) wrote: > > > Change committed as 167227 > > > > Hello. The change was been committed 1.5 years ago, but DANE still doesn't > work > > in recent chromium. Why? :( > > This change was only landed to save it in the code history. All DNSSEC stapling > support was removed from Chrome. Ок. But why it was been removed?
Message was sent while issue was closed.
On 2014/05/15 07:59:36, xaionaro wrote: > Ок. But why it was been removed? It was an experiment. It was a lot of code outside of the sandbox, ~nobody used it and it blows a hole in our efforts to remove 1024-bit RSA and to strengthen the CA system.
Message was sent while issue was closed.
On 2014/05/15 13:22:39, agl wrote: > On 2014/05/15 07:59:36, xaionaro wrote: > > Ок. But why it was been removed? > > It was an experiment. It was a lot of code outside of the sandbox, I see. > ~nobody used it I was going to use it. And still hope... > and it blows a hole in our efforts to remove 1024-bit RSA and to strengthen > the CA system. I'd prefer to trust DANE instead of PKI. Sorry for annoying, but will DANE support be enable in near future (2015-2016)?
Message was sent while issue was closed.
On 2015/01/23 04:49:15, xaionaro wrote: > Sorry for annoying, but will DANE support be enable in near future (2015-2016)? Afraid not. See https://www.imperialviolet.org/2015/01/17/notdane.html
Message was sent while issue was closed.
On 2015/01/23 19:23:13, agl wrote: > On 2015/01/23 04:49:15, xaionaro wrote: > > Sorry for annoying, but will DANE support be enable in near future > (2015-2016)? > > Afraid not. See https://www.imperialviolet.org/2015/01/17/notdane.html Any thoughts about that : https://www.ietf.org/mail-archive/web/dane/current/msg07971.html ? tl;dr: Stapling DANE/TLSA answers (in the same way OCSP can be stapled), so no more additional network request, one of the main concern of notdane. https://tools.ietf.org/html/draft-shore-tls-dnssec-chain-extension-01
Message was sent while issue was closed.
On 2016/07/19 10:34:05, tdelmas wrote: > On 2015/01/23 19:23:13, agl wrote: > > On 2015/01/23 04:49:15, xaionaro wrote: > > > Sorry for annoying, but will DANE support be enable in near future > > (2015-2016)? > > > > Afraid not. See https://www.imperialviolet.org/2015/01/17/notdane.html > > Any thoughts about that : > https://www.ietf.org/mail-archive/web/dane/current/msg07971.html ? > > tl;dr: Stapling DANE/TLSA answers (in the same way OCSP can be stapled), so no > more additional network request, one of the main concern of notdane. > > https://tools.ietf.org/html/draft-shore-tls-dnssec-chain-extension-01 Our original implementation did exactly this. No, the concerns were not addressed - the poor crypto, the poor key management, the poor centralization of control. But this is not a discussion for a code review |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
