Closed Bug 956866 Opened 6 years ago Closed 3 years ago

SSL alert callback so client can log warnings such as no_renegotiation

Categories

(NSS :: Libraries, enhancement)

3.15.3.1
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: chris.newman, Assigned: edewata)

References

Details

Attachments

(4 files, 11 obsolete files)

6.67 KB, patch
ekr
: review+
Details | Diff | Splinter Review
7.52 KB, patch
Details | Diff | Splinter Review
5.93 KB, patch
ekr
: review+
Details | Diff | Splinter Review
522 bytes, patch
kaie
: review+
Details | Diff | Splinter Review
When an NSS client receives an SSL warning alert, the information about that alert is silently discarded unless it's a debug build and NSSTRACE is enabled at the appropriate level. In the case of a no_renegotiation error, the SSL warning alert is the only pertinent information the client gets about why the handshake failed. A callback would allow the client to log this information and/or present a correct error message. This request is related to but different from bug 112051 and bug 529450. A callback could allow a client author using NSS to work around those two other bugs.
Attached patch nss-ssl-alert-callback.patch (obsolete) — Splinter Review
A new SSL_AlertHook function has been added to register an
SSLAlertHandler callback function which will be invoked when
NSS sends out an SSL alert.
Attachment #8843495 - Flags: review?(rrelyea)
Please don't replicate all the protocol code point definitions. Move them into the public API.
Comment on attachment 8843495 [details] [diff] [review]
nss-ssl-alert-callback.patch

Review of attachment 8843495 [details] [diff] [review]:
-----------------------------------------------------------------

OK, given Eric's comments, I see an other thing we need to lock down: We need to make sure the enum is the proper size on all platforms. Currently all enum values fit within a char, I believe the compiler allocates the enums as sizeof int in any case, but since this is an ABI we need to maintain, we probably need to force the issue.

I see the following way forward:

1) We could declare SSLAlertTypes and SSLAlertLevels as  unsigned ints, and not export the protocol definitions right now. The actual values are the protocol values, so your application have to deal with values which you don't recognize anyway (NSS will be updated with new alerts and your application needs to continue to run.

Later we can supply a patch which exports these values public ally.

2) We can take the hit to do the massive rename through the SSL code now.

I like the idea of providing a callback to the application on sending alerts (particularly for logging purposes), but we may not have the runway to get this all done.


Also, since this is a public API, it gets more scrutiny because we can't change it once we include it, so I think we need another parameter to handle the receive alert versus send alert case, even if we don't implement the receive alert initially.

::: lib/ssl/ssl.h
@@ +865,5 @@
> +    SSL_NO_APPLICATION_PROTOCOL = 120,
> +
> +    /* invalid alert */
> +    SSL_NO_ALERT = 256
> +} SSLAlertType;

Please head Eric's comments. These two data structures is the code he's talking about.

Rather than copying this to ssl.h, you need to also delete it from ssl3proto.h (and probably put a pointer back to ssl.h)

I see that you've renamed everything (which makes since now that your are exporting it. We need to rationalize the names somehow, probably adopting the global names in the current code.

You can alias the enums. (typedef SSLAlertType SSL3AlertDescription;)

The important thing here is we don't want to modify more than one place when new alerts are added.

@@ +867,5 @@
> +    /* invalid alert */
> +    SSL_NO_ALERT = 256
> +} SSLAlertType;
> +
> +typedef SECStatus(PR_CALLBACK *SSLAlertHandler)(void *arg, PRFileDesc *fd,

Here's where the other parameter to say if we are sending or receiving goes

::: lib/ssl/ssl3con.c
@@ +3156,5 @@
>      }
> +
> +    if (rv == SECSuccess && ss->handleAlert) {
> +        rv = (*ss->handleAlert)(ss->alertArg, ss->fd,
> +            (SSLAlertLevel)level, (SSLAlertType)desc);

yeah, at this point it's clear SSLAlertLevel and SSL3AlertDescription are exactly the same thing.

Also, We should probably have a single callback that also handles alerts that we have received from our peer. That means the handleAlert function needs another parameter telling us whether we are sending or receiving the alert.
Attachment #8843495 - Flags: review?(rrelyea) → review-
New SSL_AlertReceivedHook and SSL_AlertSentHook functions have been
added to register callback functions which will be invoked when NSS
receives and sends SSL alerts, respectively.
Attachment #8843495 - Attachment is obsolete: true
Attachment #8843840 - Flags: review?(rrelyea)
Thanks for the feedback. I have incorporated the suggested changes in the new patch. Please have a look.

In this patch the SSLAlertLevel and SSLAlertType are defined as PRUint8. This will match the type currently used in SSL3_SendAlert(). The patch does not export the protocol definitions. That can be done separately later (including renaming existing code).

This patch also provides a separate callback function to handle received alerts in ssl3_HandleAlert(). I did not to use the same handler for receiving and sending alerts (using an additional parameter) because people probably will only deal with just one case, or write different code for each case anyway, so separate handlers will probably be simpler to use.
While the basic idea of the patch is correct ("use sent or received alerts to log types of failures encountered") is correct, it implicitly makes two assumptions on what the existing NSS does with regards to sent alerts, namely:
 a). It assumes that NSS will always send an alert if an error condition is encountered
 b). It assumes that the type of alert sent is always appropriate for the error at hand

I've seen both false positives (errors unrelated to negotiation of parameters reported with handshake_failure) and false negatives (errors with negotiation of parameters causing connection drop without alert).

So while this callback is _required_ to provide information to applications about failures, it's not _sufficient_ if the data returned by it is supposed to be anything but noise.
Comment on attachment 8843840 [details] [diff] [review]
0001-Bug-956866-Added-SSL-alert-callbacks.patch

Review of attachment 8843840 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rrelyea
I'd like feedback from Eric before I check this in.
Attachment #8843840 - Flags: review?(rrelyea)
Attachment #8843840 - Flags: review+
Attachment #8843840 - Flags: feedback?(ekr)
Specifically Eric, are alerts 1 byte values in the TLS protocol (and are they likely to stay 1 byte in the future).

bob
(In reply to Robert Relyea from comment #8)
> Specifically Eric, are alerts 1 byte values in the TLS protocol

yes, description and level are 1-byte values each:
RFC 5246 Section 7.2:

      enum { warning(1), fatal(2), (255) } AlertLevel;

      enum {
          close_notify(0),
          unexpected_message(10),
          ...
          (255)
      } AlertDescription;

      struct {
          AlertLevel level;
          AlertDescription description;
      } Alert;

> (and are
> they likely to stay 1 byte in the future).

given that they may be sent by server as the first, and only, message to the client, as a response to Client Hello, they need to remain backwards compatible with SSLv3 (and they have exact same definition in TLSv1.3 draft-18)
Comment on attachment 8843840 [details] [diff] [review]
0001-Bug-956866-Added-SSL-alert-callbacks.patch

Review of attachment 8843840 [details] [diff] [review]:
-----------------------------------------------------------------

Architecturally, I'm concerned about letting these hooks change success into error and vice versa. What's the use case for that?

Specifically, we ignore the return value of SSL3_SendAlert() all over the code, so this will not have the desired effect
in those cases. if you want this function, you will need a much bigger patch.

Bob, to the specific question you asked, yes, these values will continue to be the same size.

::: lib/ssl/ssl.h
@@ +825,5 @@
> +typedef PRUint8 SSLAlertLevel;
> +typedef PRUint8 SSLAlertType;
> +
> +typedef SECStatus(PR_CALLBACK *SSLAlertHandler)(void *arg, PRFileDesc *fd,
> +    SSLAlertLevel level, SSLAlertType type);

This code seems to be a bit confused about whether the void * or the fd comes first, but I think on balance it should be fd first. Also, this seems like it should be named SSLAlertCallback because its a notification. We don't do anything with it.

@@ +830,5 @@
> +SSL_IMPORT SECStatus SSL_AlertReceivedHook(PRFileDesc *fd, SSLAlertHandler f,
> +    void *arg);
> +SSL_IMPORT SECStatus SSL_AlertSentHook(PRFileDesc *fd, SSLAlertHandler f,
> +    void *arg);
> +/*

Please use a consistent name between {Hook, Callback, Handler}. The convention here (see SSLHandshakeCallback) seems like it should be Callback, but consistency is the highest priorit

::: lib/ssl/sslsock.c
@@ +2158,5 @@
> +        ss->alertReceivedArg = sm->alertReceivedArg;
> +    if (sm->alertSent)
> +        ss->alertSent = sm->alertSent;
> +    if (sm->alertSentArg)
> +        ss->alertSentArg = sm->alertSentArg;

I get that this conforms to the rest of the code, but it seems extremely odd to (for instance) have the model socket have sm->alertSent == 0 and sm->alertSentArg nonzero and overwrite only one of them in the new socet.
Attachment #8843840 - Flags: feedback?(ekr) → feedback-
Could you please add a 'const char *' parameter to the callback with a string description of the Alert? Even if the first patch sets it to NULL initially, it removes the need to add yet another API in the future that the callback would have to use to convert the numeric alert code to a string description for logging purposes.

I also see no reason for these callbacks to change the behavior. They're mostly for logging/diagnostic purposes. Thank you for working on this!
So such a string is neither needed nor readily available to the actual SSL library, so it doesn't really make sense for the library to supply it. If we every supply it it would have to be a separate function (probably in util rather than ssl).

bob
Thanks for the feedback. Please take a look at the new patch (same file name).

For consistency with SSL_HandshakeCallback(), the callback registration functions are now called SSL_AlertReceivedCallback() and SSL_AlertSentCallback().

The callback function prototype is now called SSLAlertCallback and it returns void, so the callback functions will not affect NSS execution. The parameters are also reversed.

For consistency with handshakeCallback, the callback attributes are now called alertReceivedCallback and alertSentCallback, and the data attributes are called alertReceivedCallbackData and alertSentCallbackData. If it's too long I can change it back.

In SSL_ReconfigFD() the attributes are copied without any conditions.

I also renamed SSLAlertType to SSLAlertDescription because the term 'description' is actually defined in RFC 5246 and will be more consistent with the internal SSL3AlertDescription.

Thanks!
Attachment #8843840 - Attachment is obsolete: true
Attachment #8844254 - Flags: review?(ekr)
I will try to take a look tomorrow. In the meantime, this needs tests.

We are generally trying to add them to the gtests, so that is what would
be most convenient.
It looks like gtests are disabled on Fedora due to bug #1280846.
Well, it would be good if you fixed that, but in any case, tests of some kind are needed.
The underlying issue (bug #1300109) is already fixed in version 3.30, but it's not available yet on Fedora, so right now I'm not able to write and run the tests for this feature. Do you know anybody who can help run the tests?
You should be able to do a try push. Ask on #nss
Attached is a new patch that adds some tests using selfserv and tstclnt as suggested by Bob and Kai. Please have a look. Thanks.

There's only one new scenario added to sslauth.txt, but this scenario is actually executed 16 times presumably under different conditions. The current goal is just to demonstrate that the basic callback functionality is working, not to validate the correctness of the alert level/description under various scenarios (that is more complicated to test because the same scenario may generate different alerts under different conditions).
Attachment #8844920 - Flags: review?(ekr)
The ssl.def file needs the following fix:

-;+NSS_3.30.0.1  # Additional symbols for NSS 3.30 release
+;+NSS_3.30.0.1 { # Additional symbols for NSS 3.30 release
Attached patch 956866-fix-clang-format.patch (obsolete) — Splinter Review
fix source code formatting
Attached patch 956866-combined.patch (obsolete) — Splinter Review
The try build is all green.

For your reviewing convenience, I'm attaching a combined patch, that includes all the pieces. I also uses 30 lines of context, to make reviewing easier.
It seems you've added the output to selfserv unconditionally. Given that we run stress tests, that execute many connections, it results in a very large amount of log messages (> 50.000 per cycle).

It would be nice to have a parameter for selfserv that only prints these messages in your new test scenario.

I wonder if --check-bad-certificate is the best parameter name here.
If your hook looks at TLS protocol alerts, then there are probably many scenarios that trigger an alert, without being related to a bad certificate.
Maybe a better name could be "--report-tls-alerts" ?
Comment on attachment 8844920 [details] [diff] [review]
0002-Bug-956866-Added-tests-for-SSL-alert-callbacks.patch

Review of attachment 8844920 [details] [diff] [review]:
-----------------------------------------------------------------

--check-bad-certificate in particular is far better suited to a gtest.

I have no objection to installing the callbacks and printing the alerts, that's a fine thing, but building specific test cases in this fashion only makes selfserv/tstclnt more complex and harder to maintain.
The original intent of this test is to validate the callback mechanism, not to validate the alert levels/descriptions. So I picked one scenario that's relatively easy to test, i.e. bad certificate scenario.

The sslauth.txt can only be used to validate a single return code, that's why the bad certificate test validation had to be implemented in tstclnt to check the count, level, and description of the alert received. As mentioned in comment #19, a single line in sslauth.txt generates 16 different executions. The bad certificate test works because in all those executions the code generates the same alerts. If the code generates different alerts probably the result can only be validated using gtests, but that's not the purpose of the current test.

The --report-tls-alerts implies that the tool will only display the alerts. I'm not sure how we would validate tstclnt output with the shell-based test framework.

I was originally thinking we could later expand tstclnt to support other test scenarios using --check-<scenario>, but it really depends if tstclnt is considered as a generic SSL testing tool or an NSS-specific test driver to run NSS-specific test scenarios.
Comment on attachment 8844920 [details] [diff] [review]
0002-Bug-956866-Added-tests-for-SSL-alert-callbacks.patch

Review of attachment 8844920 [details] [diff] [review]:
-----------------------------------------------------------------

::: cmd/selfserv/selfserv.c
@@ +433,5 @@
> +static void
> +myAlertSentCallback(PRFileDesc *fd, void *arg, SSLAlertLevel level, SSLAlertDescription desc)
> +{
> +    fprintf(stderr, "selfserv: alert sent: level=%d desc=%d\n", level, desc);
> +}

This test needs to test that you are sending the right # of alerts.

::: cmd/tstclnt/tstclnt.c
@@ +1056,5 @@
> +    lastAlertDescriptionReceived = 255; /* undefined */
> +
> +    alertsSent = 0;
> +    lastAlertLevelSent = 0;
> +    lastAlertDescriptionSent = 255; /* undefined */

Why are you initializing the alert values to anything The code should guarantee that if alerts{REceived,Sent} is >0 that these have valid values.

@@ +1252,5 @@
>      serverCertAuth.dbHandle = CERT_GetDefaultCertDB();
>  
>      SSL_AuthCertificateHook(s, ownAuthCertificate, &serverCertAuth);
> +    SSL_AlertReceivedCallback(s, ownAlertReceivedCallback, NULL);
> +    SSL_AlertSentCallback(s, ownAlertSentCallback, NULL);

Check for errors in these calls.

@@ +1476,5 @@
>          PR_Close(s);
>      }
>  
> +    if (checkBadCertificate) {
> +

Extra whitespace

@@ +1489,5 @@
> +        }
> +
> +        if (lastAlertDescriptionReceived != 42) { /* bad_certificate */
> +            fprintf(stderr, "tstclnt: last alert description received: %d, expected: %d\n", lastAlertDescriptionReceived, 42);
> +            return 1;

Please don't hardcode 2 and 42 here. At least make them defines in this code.

@@ +1496,5 @@
> +        if (alertsSent != 0) {
> +            fprintf(stderr, "tstclnt: alerts sent: %d, expected: %d\n", alertsSent, 0);
> +            return 1;
> +        }
> +    }

Why aren't you checking for alerts *not* being sent for the other cases?

@@ +1581,5 @@
> +    longopts[0].longOptName = "check-bad-certificate";
> +    longopts[0].longOption = 0;
> +    longopts[0].valueRequired = PR_FALSE;
> +
> +    longopts[1].longOptName = NULL;

Why don't you initialize this array in place rather than doing these assignments.

@@ +1592,5 @@
>      while ((optstatus = PL_GetNextOpt(optstate)) == PL_OPT_OK) {
> +
> +        /* long options */
> +        if (optstate->longOptIndex == 0) {
> +            checkBadCertificate = PR_TRUE;

This is a very confusing flag. It seems like it is telling the client "expect the server to check the certificate". Is that correct? Maybe you can at least generalize this so that it is like "expect alert blah"
Attachment #8844920 - Flags: review?(ekr) → review-
Comment on attachment 8844254 [details] [diff] [review]
0001-Bug-956866-Added-SSL-alert-callbacks.patch

Review of attachment 8844254 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl.h
@@ +824,5 @@
> + */
> +typedef PRUint8 SSLAlertLevel;
> +typedef PRUint8 SSLAlertDescription;
> +
> +typedef void(PR_CALLBACK *SSLAlertCallback)(PRFileDesc *fd, void *arg,

Seems like fd should be const here.

::: lib/ssl/ssl3con.c
@@ +3154,5 @@
>      if (needHsLock) {
>          ssl_ReleaseSSL3HandshakeLock(ss);
>      }
> +    if (rv == SECSuccess && ss->alertSentCallback) {
> +        (*ss->alertSentCallback)(ss->fd, ss->alertSentCallbackData, level, desc);

You shouldn't need to dereference function pointers to call them, here, and below.

::: lib/ssl/sslsock.c
@@ +2154,5 @@
>          ss->sniSocketConfigArg = sm->sniSocketConfigArg;
> +    ss->alertReceivedCallback = sm->alertReceivedCallback;
> +    ss->alertReceivedCallbackData = sm->alertReceivedCallbackData;
> +    ss->alertSentCallback = sm->alertSentCallback;
> +    ss->alertSentCallbackData = sm->alertSentCallbackData;

I'm sorry if I steered you in the wrong direction here. I think you should mimic the existing style, gross though it is. Maybe just make it dependent on the callback being set and then you copy both.
Attachment #8844254 - Flags: review?(ekr)
Please take a look at the new callback patch which incorporate the feedback from comments #20, #21, and #28. Thanks!
Attachment #8844254 - Attachment is obsolete: true
Attachment #8845634 - Flags: review?(ekr)
The old test patch has been withdrawn due to the limitation of the shell-based test framework. Please take a look at the new gtest-based test patch. Thanks!
Attachment #8844920 - Attachment is obsolete: true
Attachment #8845637 - Flags: review?(ekr)
Just a note, in the new callback patch I've added the SSLAlert struct as defined in RFC 5246. This should simplify the interface a little bit.
Comment on attachment 8845634 [details] [diff] [review]
0001-Bug-956866-Added-SSL-alert-callbacks.patch

Review of attachment 8845634 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with the changes below

::: lib/ssl/ssl.h
@@ +831,5 @@
> +    SSLAlertDescription description;
> +} SSLAlert;
> +
> +typedef void(PR_CALLBACK *SSLAlertCallback)(const PRFileDesc *fd, void *arg,
> +                                            SSLAlert alert);

I don't object to having a struct here, but passing structs around is gross, so please do const SSLAlert*

::: lib/ssl/sslimpl.h
@@ +1136,5 @@
>      void *sniSocketConfigArg;
> +    SSLAlertCallback alertReceivedCallback;
> +    void *alertReceivedCallbackData;
> +    SSLAlertCallback alertSentCallback;
> +    void *alertSentCallbackData;

These should be called "Arg" not "Data" per the style above.
Attachment #8845634 - Flags: review?(ekr) → review+
Comment on attachment 8845637 [details] [diff] [review]
0002-Bug-956866-Added-gtests-for-SSL-alert-callbacks.patch

Review of attachment 8845637 [details] [diff] [review]:
-----------------------------------------------------------------

Please also add a test that the number of alerts is 0 at the end of Connect(). You may need to restrict this to TLS 1.2 for now because early data has an alert.

::: gtests/ssl_gtest/ssl_extension_unittest.cc
@@ +176,5 @@
>      EXPECT_EQ(kTlsAlertFatal, alert_recorder->level());
>      EXPECT_EQ(alert, alert_recorder->description());
> +
> +    EXPECT_EQ(kTlsAlertFatal, server_->LastAlertLevelSent());
> +    EXPECT_EQ(alert, server_->LastAlertDescriptionSent());

You need to check count here.

::: gtests/ssl_gtest/ssl_hrr_unittest.cc
@@ +234,4 @@
>  
>  TEST_F(TlsConnectTest, Select12AfterHelloRetryRequest) {
>    EnsureTlsSetup();
> +

Spurious whitespace

::: gtests/ssl_gtest/tls_agent.h
@@ +237,5 @@
> +  }
> +
> +  void SetAlertSentCallback(AlertCallbackFunction alert_sent_callback) {
> +    alert_sent_callback_ = alert_sent_callback;
> +  }

Do you ever call these?

@@ +258,5 @@
> +  SSLAlertDescription LastAlertDescriptionReceived() const { return last_alert_received_.description; }
> +
> +  size_t AlertSentCount() const { return alert_sent_count_; }
> +  SSLAlertLevel LastAlertLevelSent() const { return last_alert_sent_.level; }
> +  SSLAlertDescription LastAlertDescriptionSent() const { return last_alert_sent_.description; }

These accessors should be:

bool GetLastAlertSent(SSLAlert* alert)

and should check for count != 0 and otherwise return false.
Attachment #8845637 - Flags: review?(ekr) → review-
Please take a look at the revised patch based on comment #32. Thanks!
Attachment #8845634 - Attachment is obsolete: true
Attachment #8845690 - Flags: review?(ekr)
Comment on attachment 8845690 [details] [diff] [review]
0001-Bug-956866-Added-SSL-alert-callbacks.patch

Review of attachment 8845690 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8845690 - Flags: review?(ekr) → review+
Please take a look at the revised patch based on comment #33. The test for CheckConnected() will be added in a separate patch. Thanks!
Attachment #8845637 - Attachment is obsolete: true
Attachment #8845691 - Flags: review?(ekr)
Comment on attachment 8845691 [details] [diff] [review]
0002-Bug-956866-Added-gtests-for-SSL-alert-callbacks.patch

Review of attachment 8845691 [details] [diff] [review]:
-----------------------------------------------------------------

::: gtests/ssl_gtest/ssl_extension_unittest.cc
@@ +168,5 @@
>  
>    void ClientHelloErrorTest(std::shared_ptr<PacketFilter> filter,
>                              uint8_t alert = kTlsAlertDecodeError) {
> +    SSLAlert alertSent;
> +    SSLAlert alertReceived;

You could probably just have one variable called alert

@@ +179,5 @@
>      EXPECT_EQ(kTlsAlertFatal, alert_recorder->level());
>      EXPECT_EQ(alert, alert_recorder->description());
> +
> +    EXPECT_EQ(0U, server_->AlertReceivedCount());
> +

What is this whitepsace for?

@@ +190,5 @@
> +    EXPECT_TRUE(client_->GetLastAlertReceived(&alertReceived));
> +    EXPECT_EQ(kTlsAlertFatal, alertReceived.level);
> +    EXPECT_EQ(alert, alertReceived.description);
> +
> +    EXPECT_EQ(0U, client_->AlertSentCount());

I tend to think you want to refactor this out into a function called
CheckAlert(TlsAgent* agent, direction, description) ...

::: gtests/ssl_gtest/tls_agent.cc
@@ +62,5 @@
>        auth_certificate_hook_called_(false),
> +      alert_received_count_(0),
> +      last_alert_received_({ 0, 0 }),
> +      alert_sent_count_(0),
> +      last_alert_sent_({ 0, 0 }),

What does clang-format make of these { 0, 0 } ?

::: gtests/ssl_gtest/tls_agent.h
@@ +245,5 @@
>    }
>  
> +  size_t AlertReceivedCount() const { return alert_received_count_; }
> +
> +  bool GetLastAlertReceived(SSLAlert *alert) const {

The * is attached to the type

@@ +246,5 @@
>  
> +  size_t AlertReceivedCount() const { return alert_received_count_; }
> +
> +  bool GetLastAlertReceived(SSLAlert *alert) const {
> +

Extra whitespace

@@ +247,5 @@
> +  size_t AlertReceivedCount() const { return alert_received_count_; }
> +
> +  bool GetLastAlertReceived(SSLAlert *alert) const {
> +
> +      if (alert == NULL || alert_received_count_ == 0) {

How can alert_ be NULL? Do you check this elsewhere?

Also, in this code, we don't compare against 0 or NULL but use !

@@ +258,5 @@
> +  }
> +
> +  size_t AlertSentCount() const { return alert_sent_count_; }
> +
> +  bool GetLastAlertSent(SSLAlert *alert) const {

See above comments

@@ +351,5 @@
>      return SECSuccess;
>    }
>  
> +  static void AlertReceivedCallback(const PRFileDesc* fd, void* arg, const SSLAlert *alert) {
> +    TlsAgent* agent = reinterpret_cast<TlsAgent*>(arg);

If you think NULLS can happen, check for NULL here.

@@ +354,5 @@
> +  static void AlertReceivedCallback(const PRFileDesc* fd, void* arg, const SSLAlert *alert) {
> +    TlsAgent* agent = reinterpret_cast<TlsAgent*>(arg);
> +
> +    fprintf(stderr, "%s: Alert received: level=%d desc=%d\n",
> +            agent->role_str().c_str(), alert->level, alert->description);

use std::cerr here.

@@ +355,5 @@
> +    TlsAgent* agent = reinterpret_cast<TlsAgent*>(arg);
> +
> +    fprintf(stderr, "%s: Alert received: level=%d desc=%d\n",
> +            agent->role_str().c_str(), alert->level, alert->description);
> +    agent->alert_received_count_++;

We do prefix ++ in this code.

@@ +359,5 @@
> +    agent->alert_received_count_++;
> +    agent->last_alert_received_ = *alert;
> +  }
> +
> +  static void AlertSentCallback(const PRFileDesc* fd, void* arg, const SSLAlert *alert) {

See above comments.
Attachment #8845691 - Flags: review?(ekr)
Please take a look at the new patch based on the feedback in comment #37. Thanks!

I'm leaving out the refactoring of the validation commands in ClientHelloErrorTest and ServerHelloErrorTest since those tests (especially the counts) seem to be very test case specific. Perhaps later when we have more test cases for alerts we can revisit this issue.

I'm not quite sure about clang-format, but the patch builds just fine.

Also as discussed, the NULL checks have been removed since they are not necessary.
Attachment #8845691 - Attachment is obsolete: true
Attachment #8845965 - Flags: review?(ekr)
This patch adds a test in CheckConnected() as described in comment #33. Please take a look. Thanks!
Attachment #8845966 - Flags: review?(ekr)
Comment on attachment 8845965 [details] [diff] [review]
0002-Bug-956866-Added-gtests-for-SSL-alert-callbacks.patch

Review of attachment 8845965 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with nits below. Please just fix this and land it.

You also should run clang-format before you land it, otherwise it will almost certainly be busted when it hits the tree.

::: gtests/ssl_gtest/tls_agent.h
@@ +246,5 @@
>  
> +  size_t AlertReceivedCount() const { return alert_received_count_; }
> +
> +  bool GetLastAlertReceived(SSLAlert* alert) const {
> +      if (!alert_received_count_) return false;

Please put this on another line and in braces, here and below.

@@ +344,5 @@
> +  static void AlertReceivedCallback(const PRFileDesc* fd, void* arg, const SSLAlert* alert) {
> +    TlsAgent* agent = reinterpret_cast<TlsAgent*>(arg);
> +
> +    std::cerr << agent->role_str()
> +              << ": Alert received: level=" << (int)alert->level

Please use C++ style casts here and below.
Attachment #8845965 - Flags: review?(ekr) → review+
Comment on attachment 8845966 [details] [diff] [review]
0003-Bug-956866-Added-SSL-alert-test-in-CheckConnected.patch

Review of attachment 8845966 [details] [diff] [review]:
-----------------------------------------------------------------

::: gtests/ssl_gtest/tls_agent.h
@@ +246,4 @@
>  
>    size_t AlertReceivedCount() const { return alert_received_count_; }
>  
> +  void SetExpectedAlertReceivedCount(size_t count) { expected_alert_received_count_ = count; }

This looks like it is over 80 columns.

@@ +246,5 @@
>  
>    size_t AlertReceivedCount() const { return alert_received_count_; }
>  
> +  void SetExpectedAlertReceivedCount(size_t count) { expected_alert_received_count_ = count; }
> +  size_t ExpectedAlertReceivedCount() const { return expected_alert_received_count_; }

This is named expectd_alert_received_count()

@@ +257,5 @@
>  
>    size_t AlertSentCount() const { return alert_sent_count_; }
>  
> +  void SetExpectedAlertSentCount(size_t count) { expected_alert_sent_count_ = count; }
> +  size_t ExpectedAlertSentCount() const { return expected_alert_sent_count_; }

This is named expected_alert_sent_count()

::: gtests/ssl_gtest/tls_connect.cc
@@ +300,5 @@
>    CheckResumption(expected_resumption_mode_);
>    client_->CheckSecretsDestroyed();
>    server_->CheckSecretsDestroyed();
> +
> +  if (client_->version() == SSL_LIBRARY_VERSION_TLS_1_2) {

Why are you restricting this to 1.2? This test needs to work for all versions.

@@ +304,5 @@
> +  if (client_->version() == SSL_LIBRARY_VERSION_TLS_1_2) {
> +    EXPECT_EQ(client_->ExpectedAlertReceivedCount(), client_->AlertReceivedCount());
> +    EXPECT_EQ(client_->ExpectedAlertSentCount(), client_->AlertSentCount());
> +    EXPECT_EQ(server_->ExpectedAlertReceivedCount(), server_->AlertReceivedCount());
> +    EXPECT_EQ(server_->ExpectedAlertSentCount(), server_->AlertSentCount());

Instead of exposing these, add a CheckAlertCount() method to agent.
Attachment #8845966 - Flags: review?(ekr) → review-
This patch has been updated based on comment #40. It also fixes an existing clang-format issue.
Attachment #8845965 - Attachment is obsolete: true
This patch has been updated based on comment #41. Please take a look. Thanks!
Attachment #8845966 - Attachment is obsolete: true
Attachment #8846194 - Flags: review?(ekr)
Comment on attachment 8846194 [details] [diff] [review]
0003-Bug-956866-Added-SSL-alert-test-in-CheckConnected.patch

Review of attachment 8846194 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM once you remove the unnecessary getters.

::: gtests/ssl_gtest/tls_agent.h
@@ +251,5 @@
> +  }
> +
> +  size_t expected_alert_received_count() const {
> +    return expected_alert_received_count_;
> +  }

This getter does not appear to be needed.

@@ +269,5 @@
> +  }
> +
> +  size_t expected_alert_sent_count() const {
> +    return expected_alert_sent_count_;
> +  }

Nor this one.
Attachment #8846194 - Flags: review?(ekr) → review+
Attachment #8845037 - Attachment is obsolete: true
Attachment #8845064 - Attachment is obsolete: true
I started a try build, using the combination of all 3 patches, and the getters from comment 44 removed:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=58403e6ba62b3471b1bce65e55bd71f640bff137
Remaining patch to make clang-format happy, as reported by nss-try.

IIUC this work is now fully reviewed.

If nobody objects, I'll check this in tomorrow to NSS trunk and NSS 3.30 branch.
try build for 3.30 branch (includes fix from comment 46).
Comment on attachment 8846448 [details] [diff] [review]
clang-956866.patch

Review of attachment 8846448 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8846448 - Flags: review?
(In reply to Kai Engert (:kaie) from comment #46)
> Created attachment 8846448 [details] [diff] [review]
> clang-956866.patch
> 
> Remaining patch to make clang-format happy, as reported by nss-try.
> 
> IIUC this work is now fully reviewed.
> 
> If nobody objects, I'll check this in tomorrow to NSS trunk and NSS 3.30
> branch.

Ship it
trunk:
https://hg.mozilla.org/projects/nss/rev/33be0a381a47

3.30 branch:
https://hg.mozilla.org/projects/nss/rev/3ed492744514
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.30
Assignee: nobody → edewata
Attachment #8846448 - Flags: review? → review+
Thanks everyone for the reviews and assistance!
Comment on attachment 8846194 [details] [diff] [review]
0003-Bug-956866-Added-SSL-alert-test-in-CheckConnected.patch

Review of attachment 8846194 [details] [diff] [review]:
-----------------------------------------------------------------

I was motivated to look at this patch when the comment was so strange.  I'm not sure that this is the right answer.

::: gtests/ssl_gtest/ssl_0rtt_unittest.cc
@@ +24,5 @@
>  
>  TEST_P(TlsConnectTls13, ZeroRtt) {
>    SetupForZeroRtt();
> +  client_->SetExpectedAlertSentCount(1);
> +  server_->SetExpectedAlertReceivedCount(1);

NSS will never send more than one alert, so why do these methods take a count?

If we are going to expect an alert, I recommend something like this:

client_->ExpectSendAlert(<alert_type>);
server_->ExpectReceiveAlert(<alert_type>);

Which you can wrap in a function in the base test class:

TlsConnectTestBase::ExpectAlert(std::shared_ptr<TlsAgent>& sender, uint8_t type) {
  std::shared_ptr<TlsAgent>& receiver = (sender == client_) ? server_ : client_;
  sender->ExpectSendAlert(type);
  receiver->ExpectSendAlert(type);
}

If you do this, then you don't need to check alerts after the fact.  In fact, a lot of code for checkout after the fact can be removed.


Does the API report close_notify?

::: gtests/ssl_gtest/ssl_version_unittest.cc
@@ +241,4 @@
>  
>  TEST_P(TlsConnectGeneric, AlertBeforeServerHello) {
>    EnsureTlsSetup();
> +  client_->SetExpectedAlertReceivedCount(1);

Why doesn't the server send an alert?

::: gtests/ssl_gtest/tls_agent.cc
@@ +65,3 @@
>        last_alert_received_({0, 0}),
>        alert_sent_count_(0),
> +      expected_alert_sent_count_(0),

The sorting here is odd.
Comment on attachment 8846194 [details] [diff] [review]
0003-Bug-956866-Added-SSL-alert-test-in-CheckConnected.patch

Review of attachment 8846194 [details] [diff] [review]:
-----------------------------------------------------------------

::: gtests/ssl_gtest/ssl_0rtt_unittest.cc
@@ +24,5 @@
>  
>  TEST_P(TlsConnectTls13, ZeroRtt) {
>    SetupForZeroRtt();
> +  client_->SetExpectedAlertSentCount(1);
> +  server_->SetExpectedAlertReceivedCount(1);

It should report close_notify, but it doesn't seem to have a test for that.

::: gtests/ssl_gtest/ssl_version_unittest.cc
@@ +241,4 @@
>  
>  TEST_P(TlsConnectGeneric, AlertBeforeServerHello) {
>    EnsureTlsSetup();
> +  client_->SetExpectedAlertReceivedCount(1);

It's injected byt he filter.
Depends on: 1348720
Blocks: 1348856
Blocks: 1360207
You need to log in before you can comment on or make changes to this bug.