887984 - NTLM: Get telemetry for usage breakdown of windows-lib vs ntlm-auth-binary vs generic

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

NTLM support in Firefox is through 1 of three options: Windows system libraries, ntlm-auth-binary on Linux, or a generic module.

We need to understand the breakdown of this library usage, how often generic is used as a percentage of all three. So, telemetry should be added to get some data on this.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Honza, can you provide some info on where Adrian should add the telemetry, please?

Comment 2

[Honza Bambas (:mayhemer)]

Here it is, including some background to understand the structure:

nsHttpChannel and WebSocketChannel are using:

  nsHttpChannelAuthProvider that, based on the auth schema received in the auth header, selects one of implementations of nsIHttpAuthenticator:

    nsHttpBasicAuth
    nsHttpDigestAuth
    nsHttpNegotiateAuth --\
    nsHttpNTLMAuth      --/-- these two are the interesting ones, they are using, based on conditions (here you want the telemetry!), a "module" derived of nsIAuthModule:

      nsAuthGSSAPI
      nsAuthSASL
      nsAuthSSPI - windows system API wrapper
      nsAuthSambaNTLM - using ntlm_auth binary of samba on linux
      nsNTLMAuthModule - this is our generic module, we implement NTLM our self here, only v1 support now

You can add telemetry probes to each module's Init() method implementation.  Each module is always newly instantiated for a single auth transaction, but please rather re-check it's really so.

Comment 3

[Adrian Lungu]

Hey,

I have a couple of questions if you can help me out with, please.
My first question is related to how often this data should be sent: should we send them for every transaction or is enough to send them only once per session. Steve suggested that we could use a static variable in the Init() method, so we won't inflate the numbers if this method is called more than once for a transaction. The drawback of this implementation would be that we will send this data only once per session and not for every transaction made. What do you think that it would be the best approach?
My second questions is related to testing. Do you know how we can test this? I've tried to find some websites that uses NTLM, but i had no success.

Thanks,
Adrian

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Adrian Lungu from comment #3)
> Hey,
> 
> I have a couple of questions if you can help me out with, please.
> My first question is related to how often this data should be sent: should
> we send them for every transaction or is enough to send them only once per
> session. Steve suggested that we could use a static variable in the Init()
> method, so we won't inflate the numbers if this method is called more than
> once for a transaction. The drawback of this implementation would be that we
> will send this data only once per session and not for every transaction
> made. What do you think that it would be the best approach?

I strongly believe that Init() method is called ones per auth transaction.  For NTLM it's at the start of a connection since NTLM is authenticating a connection, not every single request.

We should also recognize authentication to a site and to a proxy.  That is quit important information.  You have this information only at nsHttpNTLMAuth.  It doesn't propagate to the modules.  Remember that nsHttpNTLMAuth is a service.  You probably have to use nsHttpNTLMAuth::ChallengeReceived method to put the probe at.

Have an "enumerated" probe (enumerating module types) but report only ones per session a module type has been used.  So, remember you have already reported e.g. the samba module to telemetry and don't do it again.  This is probably what Steve suggests too.

I can imagine a code like:
#define SAMBA 1
static bool sReportedSamba = false;
if (!sReportedSamba) {
  mozilla::Telemetry::Accumulate(mozilla::telemetry::NTLM_AUTH_MODULE, SAMBA)
  sReportedSamba = true;
}

That's it!


> My second questions is related to testing. Do you know how we can test this?
> I've tried to find some websites that uses NTLM, but i had no success.

Testing :) yes, that is the less fun part.  It's hard.  We don't have any of our own servers that you could be testing with.  I have some basic environment, Patrick has too, I think Brian as well.  Those are however just local installations, in my case just quickly built to test at least something.

I can test your patches if you need to, tho.

> 
> Thanks,
> Adrian

Comment 5

[Adrian Lungu]

Attachment: First proposal

This is the first proposal. It send the telemetry data from the module's Init() methods. This solution do not send any data regarding the authentication's target (site / proxy).

Comment 6

[Adrian Lungu]

Attachment: Second proposal

This is the second proposal. Here, the telemetry data is sent from the nsHttpNTLMAuth service, so it includes the authentication target as well. For now, "nsHttpNTLMAuth::ChallengeReceived" is the only place where the NTLM modules are created, so it covers all the cases. The solution is not too extensible, but it send more information than the previous one.

Comment 7

[Adrian Lungu]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> (In reply to Adrian Lungu from comment #3)
> > Hey,
> > 
> > I have a couple of questions if you can help me out with, please.
> > My first question is related to how often this data should be sent: should
> > we send them for every transaction or is enough to send them only once per
> > session. Steve suggested that we could use a static variable in the Init()
> > method, so we won't inflate the numbers if this method is called more than
> > once for a transaction. The drawback of this implementation would be that we
> > will send this data only once per session and not for every transaction
> > made. What do you think that it would be the best approach?
> 
> I strongly believe that Init() method is called ones per auth transaction. 
> For NTLM it's at the start of a connection since NTLM is authenticating a
> connection, not every single request.
> 
> We should also recognize authentication to a site and to a proxy.  That is
> quit important information.  You have this information only at
> nsHttpNTLMAuth.  It doesn't propagate to the modules.  Remember that
> nsHttpNTLMAuth is a service.  You probably have to use
> nsHttpNTLMAuth::ChallengeReceived method to put the probe at.

I didn't understood exactly want you meant to say here, so I've uploaded 2 
patches (drafts):
The first one uses the modules Init() methods to gather and send the data. So far, 
this solution does not send any information about the authentication purpose and it
sends data only for the first authentication.
The second patch uses the nsHttpNTLMAuth::ChallengeReceived method. It sends
information about the module used and its purpose, but it sends the data at every
authentication. I could use the static variable here as well (like in the first patch), 
but would it be possible to use (in the same session) two different NTLM modules? 
For instance, at first authentication it uses the OS native library and at the second
authentication it uses our generic library (the OS library stopped responding)?
Could we have something like this in a real situation?

> 
> Have an "enumerated" probe (enumerating module types) but report only ones
> per session a module type has been used.  So, remember you have already
> reported e.g. the samba module to telemetry and don't do it again.  This is
> probably what Steve suggests too.
> 
> I can imagine a code like:
> #define SAMBA 1
> static bool sReportedSamba = false;
> if (!sReportedSamba) {
>   mozilla::Telemetry::Accumulate(mozilla::telemetry::NTLM_AUTH_MODULE, SAMBA)
>   sReportedSamba = true;
> }
> 
> That's it!
> 
> 
> > My second questions is related to testing. Do you know how we can test this?
> > I've tried to find some websites that uses NTLM, but i had no success.
> 
> Testing :) yes, that is the less fun part.  It's hard.  We don't have any of
> our own servers that you could be testing with.  I have some basic
> environment, Patrick has too, I think Brian as well.  Those are however just
> local installations, in my case just quickly built to test at least
> something.
> 
> I can test your patches if you need to, tho.
> 
> > 
> > Thanks,
> > Adrian

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 770563 [details] [diff] [review]
Second proposal

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

Yes, this would be the approach.. but:

::: netwerk/protocol/http/nsHttpNTLMAuth.cpp
@@ +273,5 @@
>                  // successful, |identityInvalid| is false, which will trigger
>                  // a default credentials attempt once we return.
>                  module = do_CreateInstance(NS_AUTH_MODULE_CONTRACTID_PREFIX "sys-ntlm");
> +                ntlmModuleUsed = isProxyAuth ? NTLM_MODULE_GENERIC_PROXY :
> +                    NTLM_MODULE_SAMBA_AUTH_NON_PROXY;

here it can be either SSPI or ntlm_auth (and there is also another mistake I think).

As I'm thinking of it, we could add a new flag to nsIAuthModule interface that the module is used to auth to a proxy.  It won't change IID and thus the interface will remain compatible.  Then, add telemetry to the init method of each implementation.

There is another reason, I'd personally would also be interested how negotiate auth and its modules are used.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 770561 [details] [diff] [review]
First proposal

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

As mentioned in feedback to the other patch, we can use aServiceFlags arg of the init() method to provide modules a knowledge whether used for proxy auth.  Please add telemetry also for negotiate modules.

::: extensions/auth/nsAuthSSPI.cpp
@@ +274,5 @@
>                                             &useBefore);
>      if (rc != SEC_E_OK)
>          return NS_ERROR_UNEXPECTED;
> +
> +    static bool sTelemetrySent = false;

to save few instructions, rather use a global static. (but maybe compilers optimize this already?)

::: netwerk/base/public/nsIAuthModule.idl
@@ +25,5 @@
>      const unsigned long REQ_DELEGATE = (1 << 1);
> +    
> +    const unsigned long NTLM_MODULE_SAMBA_AUTH = (1 << 2);
> +    const unsigned long NTLM_MODULE_WIN_API = (1 << 3);
> +    const unsigned long NTLM_MODULE_GENERIC = (1 << 4);

must be 0, 1, 2.

Comment 10

[Adrian Lungu]

Attachment: NTLM Telemetry patch

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 773533 [details] [diff] [review]
NTLM Telemetry patch

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

Sorry delay (busy).  Next time will be faster.

Looks very good overall!

I'm still missing probes in those other modules for Negotiate auth (negotiate-sspi and negotiate-gss suffixes).

::: extensions/auth/nsAuthSSPI.cpp
@@ +277,5 @@
> +
> +    static bool sTelemetrySent = false;
> +    if (!sTelemetrySent) {
> +      mozilla::Telemetry::Accumulate(mozilla::Telemetry::NTLM_MODULE_USED,
> +          serviceFlags | nsIAuthModule::REQ_PROXY_AUTH ? NTLM_MODULE_WIN_API_PROXY : NTLM_MODULE_WIN_API_NO_PROXY);

Nit: maybe indent as:

 mozilla::Telemetry::Accumulate(
   mozilla::Telemetry::NTLM_MODULE_USED
   serviceFlags | nsIAuthModule::REQ_PROXY_AUTH 
     ? NTLM_MODULE_WIN_API_PROXY 
     : NTLM_MODULE_WIN_API_NO_PROXY);

::: netwerk/base/public/nsIAuthModule.idl
@@ +27,5 @@
> +    /**
> +     * The authentication is required for a proxy connection.
> +     */
> +    const unsigned long REQ_PROXY_AUTH = (1 << 2);
> +    

remove white spaces.

@@ +36,5 @@
> +    const unsigned long NTLM_MODULE_SAMBA_AUTH_NO_PROXY = 1;
> +    const unsigned long NTLM_MODULE_WIN_API_PROXY = 2;
> +    const unsigned long NTLM_MODULE_WIN_API_NO_PROXY = 3;
> +    const unsigned long NTLM_MODULE_GENERIC_PROXY = 4;
> +    const unsigned long NTLM_MODULE_GENERIC_NO_PROXY = 5;

Instead NO_PROXY better could be SERVER or DIRECT or just nothing.

::: netwerk/protocol/http/nsHttpNTLMAuth.cpp
@@ +363,5 @@
>              return rv;
>          serviceName.AppendLiteral("HTTP@");
>          serviceName.Append(host);
>          // initialize auth module
> +        uint32_t req_flags = nsIAuthModule::REQ_DEFAULT;

requestFlags

@@ +366,5 @@
>          // initialize auth module
> +        uint32_t req_flags = nsIAuthModule::REQ_DEFAULT;
> +        if (isProxyAuth)
> +          req_flags |= nsIAuthModule::REQ_PROXY_AUTH;
> +        rv = module->Init(serviceName.get(), req_flags, domain, user, pass);

blank line between req_flags |=... and rv = module->...

Comment 12

[Adrian Lungu]

(In reply to Honza Bambas (:mayhemer) from comment #11)
> Comment on attachment 773533 [details] [diff] [review]
> NTLM Telemetry patch
> 
> Review of attachment 773533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry delay (busy).  Next time will be faster.
> 
> Looks very good overall!
> 
> I'm still missing probes in those other modules for Negotiate auth
> (negotiate-sspi and negotiate-gss suffixes).

sspi is here, but I've missed gss and sasl. I see that gss is another authentication service (kerberos), so I will add a new entry in our enum for it. On the other hand, sasl is not a service itself. It is a wrapper and it uses gss or sspi. If we send telemetry data from the sasl's Init() method, we will inflate the numbers (telemetry data will be send twice for an authentication operation: once from the sasl and once from the gss/sspi). Would that be ok?

> ::: extensions/auth/nsAuthSSPI.cpp
> @@ +277,5 @@
> > +
> > +    static bool sTelemetrySent = false;
> > +    if (!sTelemetrySent) {
> > +      mozilla::Telemetry::Accumulate(mozilla::Telemetry::NTLM_MODULE_USED,
> > +          serviceFlags | nsIAuthModule::REQ_PROXY_AUTH ? NTLM_MODULE_WIN_API_PROXY : NTLM_MODULE_WIN_API_NO_PROXY);
> 
> Nit: maybe indent as:
> 
>  mozilla::Telemetry::Accumulate(
>    mozilla::Telemetry::NTLM_MODULE_USED
>    serviceFlags | nsIAuthModule::REQ_PROXY_AUTH 
>      ? NTLM_MODULE_WIN_API_PROXY 
>      : NTLM_MODULE_WIN_API_NO_PROXY);
> 
> ::: netwerk/base/public/nsIAuthModule.idl
> @@ +27,5 @@
> > +    /**
> > +     * The authentication is required for a proxy connection.
> > +     */
> > +    const unsigned long REQ_PROXY_AUTH = (1 << 2);
> > +    
> 
> remove white spaces.
> 
> @@ +36,5 @@
> > +    const unsigned long NTLM_MODULE_SAMBA_AUTH_NO_PROXY = 1;
> > +    const unsigned long NTLM_MODULE_WIN_API_PROXY = 2;
> > +    const unsigned long NTLM_MODULE_WIN_API_NO_PROXY = 3;
> > +    const unsigned long NTLM_MODULE_GENERIC_PROXY = 4;
> > +    const unsigned long NTLM_MODULE_GENERIC_NO_PROXY = 5;
> 
> Instead NO_PROXY better could be SERVER or DIRECT or just nothing.
> 
> ::: netwerk/protocol/http/nsHttpNTLMAuth.cpp
> @@ +363,5 @@
> >              return rv;
> >          serviceName.AppendLiteral("HTTP@");
> >          serviceName.Append(host);
> >          // initialize auth module
> > +        uint32_t req_flags = nsIAuthModule::REQ_DEFAULT;
> 
> requestFlags
> 
> @@ +366,5 @@
> >          // initialize auth module
> > +        uint32_t req_flags = nsIAuthModule::REQ_DEFAULT;
> > +        if (isProxyAuth)
> > +          req_flags |= nsIAuthModule::REQ_PROXY_AUTH;
> > +        rv = module->Init(serviceName.get(), req_flags, domain, user, pass);
> 
> blank line between req_flags |=... and rv = module->...

Comment 13

[Adrian Lungu]

Attachment: NTLM Telemetry patch

Added telemetry for GSS (Kerberos).

Comment 14

[Adrian Lungu]

Attachment: bug-887984-fix.patch

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 777987 [details] [diff] [review]
bug-887984-fix.patch

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

r=honzab

Looks pretty good.  Thanks!

::: netwerk/base/public/nsIAuthModule.idl
@@ +22,5 @@
>       * flag may also need to be specified in order for this flag to take
>       * effect.
>       */
>      const unsigned long REQ_DELEGATE = (1 << 1);
> +    

whitespace

Comment 16

[Adrian Lungu]

Attachment: bug-887984-fix.patch

Removed the trailing whitespace and tested[1] on the try server.

[1] https://tbpl.mozilla.org/?tree=Try&rev=9f0e139607ab

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Cameron McCormack (:heycam)]

Backed out for Windows build failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/110669a3d10e

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

Comment 19

[Cameron McCormack (:heycam)]

(The red showed up on the try push in comment 16...)

Comment 20

[Adrian Lungu]

Attachment: bug-887984-fix.patch

Solved the Windows issue build [1]. It looks like the '#include "mozilla/Telemetry.h"' imports the "mozilla" namespace and the "TimeStamp" is converted to "mozilla::Timestamp", so I had to replace the 'TimeStamp*' with the 'PTimeStamp'.

[1] https://tbpl.mozilla.org/?tree=Try&rev=496675136a50

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 780686 [details] [diff] [review]
bug-887984-fix.patch

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

::: extensions/auth/nsAuthSSPI.cpp
@@ +270,5 @@
>                                             pai,
>                                             nullptr,
>                                             nullptr,
>                                             &mCred,
> +                                           useBefore);

Ehm.. so you are actually passing an uninitialized pointer here, right?

This is a basic mistake.

::TimeStamp doesn't work (not sure why).  So, I've done the following changes:

Under...

#include "nsAuthSSPI.h"

...I've added:

typedef TimeStamp MS_TimeStamp;

And then used MS_TimeStamp as the type.

Comment 22

[Adrian Lungu]

Attachment: bug-887984-fix.patch

Pointer problem fixed. [1]

[1] https://tbpl.mozilla.org/?tree=Try&rev=5762beea30f0

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 781194 [details] [diff] [review]
bug-887984-fix.patch

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

If otherwise same as the patch I've already r+'ed then go ahead and land if the try is OK (please check carefully this time before landing).

r=honzab with a comment addressed.

::: extensions/auth/nsAuthSSPI.h
@@ +25,5 @@
>  // implementation of NTLM should only be used for single-signon.  It should be
>  // avoided when authenticating over the internet since it may use a lower-grade
>  // version of password hashing depending on the version of Windows being used.
>  
> +typedef TimeStamp MS_TimeStamp;

Not a good idea to expose this in a header.

Comment 24

[Adrian Lungu]

Attachment: bug 887984 patch

Test results from the try server:
https://tbpl.mozilla.org/?tree=Try&rev=e952e9a82ba4

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6364e33d76f5

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6364e33d76f5

Comment 27

[Jason Duell]

So we're going to need to wait for Firefox 25 to ship to get useful info here (most of our NTLM users seems to not use beta/aurora).  Honza, do you think it's worth nominating this for beta so we can get that info 6 weeks sooner?  Bug 828183 is sec-high--we want to fix it ASAP.

Comment 28

[Honza Bambas (:mayhemer)]

Comment on attachment 782640 [details] [diff] [review]
bug 887984 patch

[Approval Request Comment]
This is just an added telemetry.  Baked for a long time on m-c and m-a.

Comment 29

[bhavana bajaj [:bajaj]]

Comment on attachment 782640 [details] [diff] [review]
bug 887984 patch

Assuming this has no real user impact but just help collect needed metrics and is very low risk and has baked well.Approving for beta.

Given this will land today or tomorrow morning PT it will be going in tomorrow's beta and will be released to our Beta audience on Friday, hence request for folks on the telemetry side to verify they are getting the data as expected.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Touches an IDL file. Needs ba+ to land on beta.

Comment 31

[Jorge Villalobos [:jorgev] (he/him)]

Shouldn't the IDL's UUID also be changed?

I suppose there will be no real add-on impact, but this would be the second time during the beta cycle that we break compatibility (bug 907893). Unless this is critical, I think it should wait.

Comment 32

[bhavana bajaj [:bajaj]]

Thanks for catching that Ryan, I agree with jorge in comment# 31.Although this might have been a good to have its too late to add IDL/UUId changes landed, hence taking back the approval here.

Comment 33

[Jason Duell]

Do we need a UUID bump even when we're just adding a few constants?  No new functions in the patch, so I wouldn't expect object size or vtable, etc to be any different.  I.e I don't see how this can break either JS or binary compat, but perhaps I'm missing some way that it can.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

That's Jorge's call, not mine. I'm just the messenger :)

Comment 35

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Jason Duell (:jduell) from comment #33)
> Do we need a UUID bump even when we're just adding a few constants?  No new
> functions in the patch, so I wouldn't expect object size or vtable, etc to
> be any different.  I.e I don't see how this can break either JS or binary
> compat, but perhaps I'm missing some way that it can.

I don't know the exact circumstances where an UUID bump is required, which is why I formulated it as a question.

If this change can be done without breaking binary compatibility, then I'm okay with it.

Comment 36

[Honza Bambas (:mayhemer)]

It's not true you have to change IID when only constants are added (or modified).  IID is for preserving binary compatibility when a function changes/is added/removed that doesn't happen here.  

We only add new consts here, so even js compat is preserved.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 782640 [details] [diff] [review]
bug 887984 patch

Looks like this should be reconsidered based on comment 35 and comment 36.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c2d6fe54ec13

Comment 39

[Honza Bambas (:mayhemer)]

Bhavana, Ryan, thanks! :)