946348 - Refactor DtlsIdentity fingerprint functions

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Martin Thomson [:mt:]]

My review of the patch for bug 892148 highlighted an encapsulation issue.  This is an easy fix.

Comment 1

[Martin Thomson [:mt:]]

Attachment: Refactoring DtlsIdentity interface; improving performance of call setup by an infinitesimal amount as a result.

Comment 2

[Martin Thomson [:mt:]]

I realized that this is more than a straight refactor, but no sense in doing things twice when what you need is just lying around.

Comment 3

[Adam Roach [:abr]]

Comment on attachment 8343399 [details] [diff] [review]
Refactoring DtlsIdentity interface; improving performance of call setup by an infinitesimal amount as a result.

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

r+, contingent on addressing the nits described below.

::: media/mtransport/dtlsidentity.cpp
@@ +24,5 @@
>    if (cert_)
>      CERT_DestroyCertificate(cert_);
>  }
>  
> +const std::string DtlsIdentity::HASH_ALGORITHM = "sha-256";

s/HASH_ALGORITHM/DEFAULT_HASH_ALGORITHM/

@@ +218,5 @@
> +
> +std::string DtlsIdentity::GetFormattedFingerprint(const std::string algorithm) {
> +  unsigned char digest[HASH_ALGORITHM_MAX_LENGTH];
> +  size_t digest_length;
> +  

Trailing WS

@@ +224,5 @@
> +                                          digest,
> +                                          sizeof(digest),
> +                                          &digest_length);
> +  if (NS_FAILED(res)) {
> +    MOZ_MTLOG(ML_ERROR, "Unable to compute " << algorithm 

Trailing WS

@@ +225,5 @@
> +                                          sizeof(digest),
> +                                          &digest_length);
> +  if (NS_FAILED(res)) {
> +    MOZ_MTLOG(ML_ERROR, "Unable to compute " << algorithm 
> +              << " hash for identity: " << static_cast<uint32_t>(res));

nsresult is almost always hex formatted. I don't know of a good, universally available way to print it, but at a bare minimum, you'll want to do something like this:

> #include <iomanip>
> 
> ...
>
>          << "nsresult = 0x" 
>          << std::hex << std::uppercase 
>          << static_cast<uint32_t>(res) 
>          << std::nouppercase << std::dec);

::: media/mtransport/dtlsidentity.h
@@ +32,5 @@
>    CERTCertificate *cert() { return cert_; }
>    SECKEYPrivateKey *privkey() { return privkey_; }
>  
> +  std::string GetFormattedFingerprint();
> +  std::string GetFormattedFingerprint(const std::string algorithm);

There isn't any reason to copy the string in here; use a const reference.

Also, it's not clear from the header that these functions do the same thing; consider collapsing them together:

std::string GetFormattedFingerprint(const std::string &algorithm = DEFAULT_HASH_ALGORITHM);

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1177,5 @@
>  /*
>   * Get DTLS key data
>   *
>   *  @param[in]  peerconnection - the peerconnection in use
>   *  @param[out] digest_algp    - the digest algorithm e.g. 'SHA-1'

Given that sha-1 is now deprecated, let's change this example to "sha-256" (note the lowercasing, since that's what we appear to be returning).

@@ +1204,5 @@
>      return VCM_ERROR;
>    }
>  
> +  sstrncpy(digest_algp, fp.substr(0, spc).c_str(), max_digest_alg_len);
> +  sstrncpy(digestp, fp.c_str() + spc + 1, max_digest_len);

This is a bit ungainly. I would suggest some means of getting these two different elements directly from the PC (e.g., "GetFingerprintAlgorithm()" and "GetFingerprintValue()")

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +244,5 @@
>    }
>  
>    // Get the DTLS identity
> +  mozilla::RefPtr<DtlsIdentity> const GetIdentity() const;
> +  std::string GetFingerprint() const;

const std::string &GetFingerprint() const;

Comment 4

[Martin Thomson [:mt:]]

(In reply to Adam Roach [:abr] from comment #3)

> nsresult is almost always hex formatted. I don't know of a good, universally
> available way to print it, but at a bare minimum, you'll want to do
> something like this:

Puke.  OK.

> std::string GetFormattedFingerprint(const std::string &algorithm =
> DEFAULT_HASH_ALGORITHM);

That's what I wanted.

> @@ +1204,5 @@
> >      return VCM_ERROR;
> >    }
> >  
> > +  sstrncpy(digest_algp, fp.substr(0, spc).c_str(), max_digest_alg_len);
> > +  sstrncpy(digestp, fp.c_str() + spc + 1, max_digest_len);
> 
> This is a bit ungainly. I would suggest some means of getting these two
> different elements directly from the PC (e.g., "GetFingerprintAlgorithm()"
> and "GetFingerprintValue()")

If I'm going to fix anything, probably not that.  I'd start with sipcc.  Ungainly is requesting that values be split into component parts, but to insist on a very particular string format for those parts rather than use a sensible structure, like say std::string + size_t + uint8_t*.

I guess I could start over, create a new "Fingerprint" class, move the string formatting there from DtlsIdentity, use that as a return value from PeerConnectionImpl, change everything that depended on it to use the class sensibly, including sipcc.  That would be the right thing to do.

Would that be what you are suggesting?  It didn't seem like it would be worthwhile.

Comment 5

[Martin Thomson [:mt:]]

Attachment: Refactoring DtlsIdentity interface; improving performance of call setup by an infinitesimal amount as a result.

Comment 6

[Martin Thomson [:mt:]]

Comment on attachment 8344933 [details] [diff] [review]
Refactoring DtlsIdentity interface; improving performance of call setup by an infinitesimal amount as a result.

Adam, I'll let you decide how to proceed here.  I've addressed the nits aside from the big one.  I don't know if you want r? or checkin? at this point.

Comment 7

[Martin Thomson [:mt:]]

Attachment: Refactoring fingerprint handling

Comment 8

[Martin Thomson [:mt:]]

Attachment: Refactoring fingerprint handling

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8349678 [details] [diff] [review]
Refactoring fingerprint handling

This should make Adam happier.  VcmSIPCCBinding gets less complex.  I am disappointed that this is a net global loss of efficiency, but...the net effect should be minimal, if not zero.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8349678 [details] [diff] [review]
Refactoring fingerprint handling

BTW, you can just use the checkin-needed bug keyword when you need landing assistance. It's a bit easier to work with.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cffe18264682

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/13dc0f84907e

https://tbpl.mozilla.org/php/getParsedLog.php?id=32224541&tree=Mozilla-Inbound

Comment 13

[Martin Thomson [:mt:]]

Attachment: Bug-946348-Refactoring-fingerprint-handling.patch

Comment 14

[Martin Thomson [:mt:]]

Attachment: Bug-946348-Refactoring-fingerprint-handling.patch

Comment 15

[Martin Thomson [:mt:]]

Attachment: Refactoring fingerprint handling

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Adam, is this good to go?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfffcc0a895

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebfffcc0a895