RTCCertificate::Expires() and ::HasExpired() implemented

webrtc/base/rtccertificate_unittests.cc

+/*
+ *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/base/checks.h"

[torbjorng (webrtc), 2015/12/04 12:47:46]

nit: I don't think all these #Includes are needed.

+#include "webrtc/base/gunit.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/base/rtccertificate.h"
+#include "webrtc/base/safe_conversions.h"
+#include "webrtc/base/scoped_ptr.h"
+#include "webrtc/base/sslidentity.h"
+#include "webrtc/base/thread.h"
+#include "webrtc/base/timeutils.h"
+
+namespace rtc {
+
+class RTCCertificateTest : public testing::Test {
+ public:
+  RTCCertificateTest() {}
+  ~RTCCertificateTest() {}
+
+ protected:
+  // Gets the current time in ms since epoch, 1970-01-01T00:00:00Z.
+  // If |seconds_precision| is true it is rounded down to seconds-precision.
+  // This is useful when comparing expiration times since CreateCertificate is
+  // limited to seconds-precision.
+  uint64_t Now(bool seconds_precision) const {
+    uint64_t now = TimeNanos() / kNumNanosecsPerMillisec;
+    if (seconds_precision)
+      return (now / 1000) * 1000;
+    return now;
+  }
+
+  // Generates a certificate with the specified |expiration_time|, expressed in
+  // number of ms since epoch, 1970-01-01T00:00:00Z. The expiration time is

[torbjorng (webrtc), 2015/12/04 12:47:46]

Note that a cert cannot store greater accuracy than 1 s. See RFC 5280.

I really don't like that we mess around with ms for expiration, since this can
only lead to confusion which may lead to failure to expire.

I realise that the WebRTC draft defines expiration in ms.

+  // converted to time_t meaning it is rounded down to number of seconds (note:
+  // loss of precision) and these number of seconds must be in valid range of
+  // time_t. On some 32-bit systems time_t is limited to < 2^31. On such systems
+  // this will fail for expiration times of year 2038 or later.
+  scoped_refptr<RTCCertificate> CreateCertificate(uint64_t expires_time) const {
+    uint64_t expires_time_s = expires_time / kNumMillisecsPerSec;  // ms -> s
+    RTC_CHECK(rtc::IsValueInRangeForNumericType<time_t>(expires_time_s));
+
+    rtc::SSLIdentityParams params;
+    params.common_name = "RTCCertificateTest's certificate";
+    params.not_before = 0;
+    params.not_after = static_cast<time_t>(expires_time_s);
+    // Certificate type does not matter for our purposes, using ECDSA because it
+    // is fast to generate.
+    params.key_params = rtc::KeyParams::ECDSA(rtc::EC_NIST_P256);
+
+    scoped_ptr<SSLIdentity> identity(rtc::SSLIdentity::GenerateForTest(params));

[hta-webrtc, 2015/12/03 18:16:05]

You can make the test a lot more isolated (not depend on rtc::SSLIdentity) if
you use a mock certificate. You're not testing rtc::SSLIdentity here, you're
testing the handling code.

[hbos, 2015/12/04 09:58:56]

Ok. I'll add a expiration setter to FakeSSLCertificate and use that.

+    return RTCCertificate::Create(identity.Pass());
+  }
+};
+
+TEST_F(RTCCertificateTest, ExpiresNow) {
+  uint64_t now = Now(true);
+  scoped_refptr<RTCCertificate> certificate = CreateCertificate(now);
+  EXPECT_EQ(now, certificate->Expires());
+}
+
+TEST_F(RTCCertificateTest, HasExpired) {
+  scoped_refptr<RTCCertificate> certificate;
+
+  // Test that a certificate that should not have expired hasn't.
+  bool retry = true;
+  while (retry) {

[hta-webrtc, 2015/12/03 18:16:05]

Looping until the times align is a Bad Thing.

You have no idea how many times you will go around the loop.
If you mock the certificate, you have much more precise control.

[hbos, 2015/12/04 09:58:56]

Point taken, I'll simplify.

Though as-is, one test run takes but milliseconds, so the odds of having to try
again is very small. If you do try again you'll no longer be "on the edge"
between second 1 and second 2 so the odds of having to try yet another time is
exceedingly unlikely. But even if this could run for a 100 times that wouldn't
be a performance problem.

That being said if there is a regression and the assumption that one attempt is
fast no longer holds we could get stuck in some undesirable looping.

+    // Generate a certificate that expires in 0-1 seconds.
+    // Note seconds-precision of |expires| due to CreateCertificate.
+    uint64_t expires = Now(true) + 1000;
+    certificate = CreateCertificate(expires);
+    bool has_expired = certificate->HasExpired();
+    if (has_expired) {
+      // Expecting not to have expired, but in the rare case that too much time
+      // passed between certificate generation and the HasExpired check we have
+      // to try again.
+      uint64_t now = Now(false);
+      if (now >= expires)
+        continue;
+    }
+    EXPECT_FALSE(has_expired);
+    retry = false;
+  }
+  RTC_CHECK(certificate);
+
+  // Test that ~1.5 seconds later it has expired.
+  Thread::Current()->SleepMs(1500);

[hta-webrtc, 2015/12/03 18:16:05]

Argh. This is going to cost us heavily. Sleeping in tests is a Bad Thing - it
wastes 1.5 seconds every time the tests are run. Good unit tests finish in
milliseconds.

I think this test could be much simpler - generate two certificates, one that
expires at now-1000 and one that expires at now+1000, check that one has expired
and one is not.

There's a risk that the future-expired certificate will be flaky - you never
know how much time will pass between any two lines in a test - but 1000 seconds
should be enough to make it not-flaky.
(Injectable time would make it not-flaky too, of course).

[hbos, 2015/12/04 09:58:56]

Ok. I wanted to test going from non-expired to expired but I guess sleeping is
not worth it. FakeSSLCertificate with expiration injection it is.

[torbjorng (webrtc), 2015/12/04 12:47:46]

I disagree (with the Argh and with the technical contents).

My view is that tests which run as close to reality are better than those that
take artificial approaches. For certificates, we will generate them, then they
will be valid and we should recognise them as valid, and later they will expire
and we should recognise them as expired.

Generating expired certificates and certificates valid longer than we can check
for the expiration to happen will not make adequate tests of the mechanism which
we need to test.

I'd suggest a slight modification to hbos' original code, but the code was fine
already. My suggestion is that one does not loop in the case the cert is expired
directly after creation, at least not indefinitely.  Instead just skip this
test. As a complement, one should generate a certificate with say 100 seconds
validity and fail if that is expired.

This will never be flaky. It will make sure that no second/millisecond confusion
exists or is introduced, and it will verify that the expiration mechanism is
sound.

This test is important enough that it is worth 1.5 seconds of wall clock time.
We already spend several minutes of CPU time on testing the crypto code.

[hta-webrtc, 2015/12/04 13:32:44]

I'm afraid I disagree about the importance. This is an unit test for code that,
in itself, has no state; the tests are already longer than the code it's
testing.

The test as written now tests that a certificate with an old expiry date shows
as expired and that a certificate with an expiry date in the future shows as not
expired; I think that's what we want to test.

A real test that the same certificate will show as not-expired before some time
and expired after that time is simple to write - but it requires an injected
clock that is under the test's control.

In my opinion, developers should run the unit tests for their module every time
they make a change to that module, almost as an instinctive action. Each second
spent extra here is a second of programmer's time lost.

Some modules require, as you say, minutes of CPU time. Those are large tests,
not unit tests. We won't ask programmers to run those on every change to their
module while they're developing.

https://wiki.corp.google.com/twiki/bin/view/Main/GoogleTestDefinitions#Defini...
is old, but good.

This is an unit test, and should be a small test.

+  EXPECT_TRUE(certificate->HasExpired());
+}
+
+}  // namespace rtc