Closed Bug 1368107 Opened 3 years ago Closed 3 years ago

Slightly improve TransportSecurityInfo API

Categories

(Core :: Security: PSM, enhancement, P1, minor)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Details

(Whiteboard: [psm-assigned])

Attachments

(4 files)

There's some low-hanging fruit for improving the API of TransportSecurityInfo that we can take care of.
Comment on attachment 8871838 [details]
Bug 1368107 - Remove TransportSecurityInfo::GetHostNameRaw().

https://reviewboard.mozilla.org/r/143322/#review147126

LGTM.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1403
(Diff revision 1)
>  
>    UniqueCERTCertList resultChain;
>    SECOidTag evOidPolicy;
>    mozilla::pkix::Result result;
>  
>    const nsCString& flatHostname = PromiseFlatCString(aHostname);

Unless I'm missing something, I think this is now only used in the else branch (i.e. where we call VerifyCert), so let's move it there.
Attachment #8871838 - Flags: review?(dkeeler) → review+
Comment on attachment 8871839 [details]
Bug 1368107 - Remove fallible version of TransportSecurityInfo::GetPort().

https://reviewboard.mozilla.org/r/143324/#review147128

Awesome.
Attachment #8871839 - Flags: review?(dkeeler) → review+
Comment on attachment 8871840 [details]
Bug 1368107 - Make some TransportSecurityInfo nsresult functions return void.

https://reviewboard.mozilla.org/r/143326/#review147130

Yep. Cool.
Attachment #8871840 - Flags: review?(dkeeler) → review+
Comment on attachment 8871841 [details]
Bug 1368107 - Make SSLErrorMessageType an enum class.

https://reviewboard.mozilla.org/r/143328/#review147132

This seems reasonable. What would be really awesome, though, is if we could get rid of the need to cache the error message on TransportSecurityInfo in the first place. It should be possible to do something like add a function to nsINSSErrorsService that takes an nsITransportSecurityInfo and returns what nsITransportSecurityInfo.errorMessage returns today, but without taking up the storage space. Once we have that, I don't think we'd even need a SSLErrorMessageType type. (Another aspect of this might involve removing the error logging or moving it to somewhere in necko.)
Attachment #8871841 - Flags: review?(dkeeler) → review+
Comment on attachment 8871841 [details]
Bug 1368107 - Make SSLErrorMessageType an enum class.

https://reviewboard.mozilla.org/r/143328/#review147132

I'll file a follow-up bug and probably look into this as well.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c263f45e41ce
Remove TransportSecurityInfo::GetHostNameRaw(). r=keeler
https://hg.mozilla.org/integration/autoland/rev/8d8bd769125e
Remove fallible version of TransportSecurityInfo::GetPort(). r=keeler
https://hg.mozilla.org/integration/autoland/rev/69bac2d5d3bd
Make some TransportSecurityInfo nsresult functions return void. r=keeler
https://hg.mozilla.org/integration/autoland/rev/5b49816e5323
Make SSLErrorMessageType an enum class. r=keeler
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.