Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(943)

Issue 302410043: Bug 1287279 - TLS 1.3: Configure RSA server certs with PKCS#1 v1.5 signatures to be available for R… (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 7 months ago by ttaubert
Modified:
7 years, 7 months ago
Reviewers:
mt
Visibility:
Public.

Description

Bug 1287279 - TLS 1.3: Configure RSA server certs with PKCS#1 v1.5 signatures to be available for R…

Patch Set 1 #

Total comments: 9

Patch Set 2 : addressed mt's review comments #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -37 lines) Patch
M external_tests/ssl_gtest/libssl_internals.h View 1 chunk +1 line, -0 lines 0 comments Download
M external_tests/ssl_gtest/libssl_internals.c View 1 chunk +11 lines, -0 lines 0 comments Download
M external_tests/ssl_gtest/ssl_auth_unittest.cc View 1 1 chunk +68 lines, -0 lines 0 comments Download
M external_tests/ssl_gtest/tls_agent.h View 1 2 chunks +4 lines, -2 lines 0 comments Download
M external_tests/ssl_gtest/tls_agent.cc View 1 3 chunks +11 lines, -6 lines 0 comments Download
M lib/ssl/sslcert.c View 1 1 chunk +71 lines, -29 lines 4 comments Download

Messages

Total messages: 5
ttaubert
https://codereview.appspot.com/302410043/diff/1/external_tests/ssl_gtest/ssl_auth_unittest.cc File external_tests/ssl_gtest/ssl_auth_unittest.cc (right): https://codereview.appspot.com/302410043/diff/1/external_tests/ssl_gtest/ssl_auth_unittest.cc#newcode360 external_tests/ssl_gtest/ssl_auth_unittest.cc:360: &ServerCertDataRsaPkcs1Decrypt)); I wonder, should this actually work? If we ...
7 years, 7 months ago (2016-08-09 21:52:09 UTC) #1
mt
I think that we need to tweak ssl_ConfigCertByUsage(). https://codereview.appspot.com/302410043/diff/1/external_tests/ssl_gtest/ssl_auth_unittest.cc File external_tests/ssl_gtest/ssl_auth_unittest.cc (right): https://codereview.appspot.com/302410043/diff/1/external_tests/ssl_gtest/ssl_auth_unittest.cc#newcode349 external_tests/ssl_gtest/ssl_auth_unittest.cc:349: TEST_F(TlsConnectTest, ...
7 years, 7 months ago (2016-08-10 00:22:20 UTC) #2
ttaubert
https://codereview.appspot.com/302410043/diff/1/external_tests/ssl_gtest/ssl_auth_unittest.cc File external_tests/ssl_gtest/ssl_auth_unittest.cc (right): https://codereview.appspot.com/302410043/diff/1/external_tests/ssl_gtest/ssl_auth_unittest.cc#newcode349 external_tests/ssl_gtest/ssl_auth_unittest.cc:349: TEST_F(TlsConnectTest, ConfigureCertRsaPkcs1SignAndKEX) { On 2016/08/10 00:22:20, mt wrote: > ...
7 years, 7 months ago (2016-08-10 12:26:12 UTC) #3
mt
LGTM https://codereview.appspot.com/302410043/diff/40001/lib/ssl/sslcert.c File lib/ssl/sslcert.c (right): https://codereview.appspot.com/302410043/diff/40001/lib/ssl/sslcert.c#newcode420 lib/ssl/sslcert.c:420: extra blank line https://codereview.appspot.com/302410043/diff/40001/lib/ssl/sslcert.c#newcode423 lib/ssl/sslcert.c:423: memcpy(&arg, data, sizeof(arg)); ...
7 years, 7 months ago (2016-08-12 05:12:44 UTC) #4
ttaubert
7 years, 7 months ago (2016-08-12 05:53:14 UTC) #5
https://codereview.appspot.com/302410043/diff/40001/lib/ssl/sslcert.c
File lib/ssl/sslcert.c (right):

https://codereview.appspot.com/302410043/diff/40001/lib/ssl/sslcert.c#newcode420
lib/ssl/sslcert.c:420: 
On 2016/08/12 05:12:44, mt wrote:
> extra blank line

Removed.

https://codereview.appspot.com/302410043/diff/40001/lib/ssl/sslcert.c#newcode423
lib/ssl/sslcert.c:423: memcpy(&arg, data, sizeof(arg));
On 2016/08/12 05:12:44, mt wrote:
> We now create three copies of this struct.  Maybe you can remove const from
the
> argument list and just modify the argument in place for this one.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b