wamp-proto / wamp-proto

The Web Application Messaging Protocol
https://wamp-proto.org/
474 stars 100 forks source link

WAMP-SCRAM specification #135

Closed sehoffmann closed 6 years ago

sehoffmann commented 9 years ago

As discussed in https://github.com/tavendo/WAMP/issues/128 WAMP-CRA is unsafe in regards to database thefts. A new and better authentication method is required. SCRAM turned out to be very promising.

oberstet already gave a summary (https://github.com/tavendo/WAMP/issues/128#issuecomment-74271094):

From looking at SCRAM, this claims to provide protection against these attack vectors:

Eavesdropping Replay Database Compromise and also against dictionary attacks and password collisions.

It additionally claims to protect against impersonating a server (after the original server had signed up a user).

Here are some references:

http://en.wikipedia.org/wiki/Salted_Challenge_Response_Authentication_Mechanism https://tools.ietf.org/html/rfc5802 http://www.mongodb.com/blog/post/improved-password-based-authentication-mongodb-30-scram-explained-part-1 http://www.mongodb.com/blog/post/improved-password-based-authentication-mongodb-30-scram-explained-part-2 The good news is, with the WAMP messages HELLO, CHALLENGE, AUTHENTICATE and WELCOME (and the extensibility built in those messages), we have the required message flow in place without changing WAMP.

However, I don't think it makes sense to put this into WAMP-CRA as it exists.

I think we should come up with a mapping of SCRAM to WAMP: WAMP-SCRAM (wamp-scram)

This should be pretty straight-foward, and we can reuse an already designed, scrutenized scheme.

We also do not need new crypto primitives, as the following will do fine:

SHA256 HMAC PBKDF2 XOR

oberstet commented 9 years ago

FWIW, I am +1 on this, and want to come up with a proof-of-concept implementation "soon". That means: AutobahnJS, AutobahnPython and Crossbar.io.

Once we have it, WAMP-SCRAM should always be chosen over WAMP-CRA, as the former is expected to provide more security than the latter.

@Paranaix The most important point is the mapping to WAMP messages. You mentioned in the other thread that reusing existing message exchange would be "misuse", because in WELCOME the client does not yet know which auth method the server chooses, but still has to send some SCRAM specific data already. Yes, that's true. However: the number of required roundtrips for establishing a session does matter in some situations. E.g. imagine widget that tracks activity on old-school Web pages. The user will navigate around, loading new pages all time, which means establishing new WAMP sessions all the time. If the connection runs over TLS, there are a lot round-trips happening even before the WAMP opening handshake begins. Another argument: adding some new meaning to existing message details does not change protocol state machines in existing WAMP implementations. Adding a multi-round CHALLENGE/AUTHENTICATE message exchange in between HELLO and WELCOME does change that. A far more invasive change.

sehoffmann commented 9 years ago

In the case additional fields are allowed for HELLO and WELCOME, I would opt for them being completely generic. While e.g a succesful WAMP-SCRAM authentication would still require certains properties to be present, additional ones shouldn't cause an error but should be simply ignored. This does not only simplify the implementation but it also makes everything more flexible and e.g allows user code to attach additional information on succesful authentication (for example a session token).

oberstet commented 8 years ago

http://www.isode.com/whitepapers/scram.html http://www.isode.com/whitepapers/strong-auth.html

ecorm commented 6 years ago

You might also want to consider the Secure Remote Password protocol.

ecorm commented 6 years ago

If my interpretation of the SCRAM RFC is correct, it should be possible to strengthen standard SCRAM-SHA-1 or SCRAM-SHA-256 by pre-hashing the password client-side with a stronger algorithm such as bcrypt, scrypt, or Argon2.

With SCRAM, the hashing work factor is limited by the client's computing power. This is nice for reducing server workload, but has the downside that the password hash strength is limited by the client. These benchmarks published on browser-computed scrypt and Argon2 hashes (using asm.js code compiled for the browser) tell us that the Javascript implementations are roughly an order of magnitude slower than their native counterparts. What would normally take 100ms on a native implementation would seem to take on the order of 1-2 seconds on an asm.js implementation for an equivalent password hash strength.

SCRAM requires that Unicode username/password strings be normalized via the SASLprep profile (RFC4013) of the stringprep framework (RFC3454). I managed to find the reklatsmasters/saslprep Javascript library, and the author confirmed that it works on modern browsers. I also found eloquent/precis-js, a JavaScript implementation of the PRECIS framework RFC7564 + RFC7613, where PRECIS supersedes stringprep. Alas, RFC7564+RFC7613 have been recently superseded by RFC8264+RFC8265.

SASLprep preserves the case of usernames, whereas PRECIS has an optional profile for mapping the username to lowercase. In the case of SASLprep, it's not clear if the server can still choose to do a case-insensitive comparison of the username in the database.

One way to sidestep normalization would be to require that usernames/passwords can only be 7-bit ASCII. This seems to be permitted under SCRAM RFC 5802, section 2.2, Normalize:

Note that implementations MUST either implement SASLprep or disallow use of non US-ASCII Unicode codepoints in "str".

Another approach might be to adopt only NFKC Unicode normalization for username/password, which is supported on the browser by String.prototype.normalize(), with unorm as a polyfill. However, this approach would ignore characters prohibited by SASLprep, such as non-ASCII spaces, or control characters.

ecorm commented 6 years ago

I've started working on a WAMP-SCRAM spec Markdown document on my personal fork. I'll let you folks know when there is enough meat in it for review.

ecorm commented 6 years ago

Here is an initial WAMP-SCRAM draft on my personal fork. I'd like to get some initial feedback before submitting a pull request.

konsultaner commented 6 years ago

@ecorm I really like the initial definition. I will soon start to implement it based on your description and give you feedback if something is not clear. @oberstet Is there a client that already supports WAMP-SCRAM?

ecorm commented 6 years ago

Thanks for your offer of feedback, @konsultaner .

Someone needs to review the algorithms in my draft and compare it with RFC5802. RFC7677 has test vectors that you can use to check your implementation if you use the same test nonces.

If a client allows you to send arbitrary HELLO.Options, lets you receive arbitrary CHALLENGE.Details, and lets you send arbitrary AUTHENTICATE.Details, you should be able to use that client.

If your client is a web browser, the WebCryptoAPI provides native implementations of PBKDF2, SHA-256 and HMAC if you're in a secure context (i.e. HTTPS).

ecorm commented 6 years ago

While writing the WAMP-SCRAM spec, I was also studying SRP, and I believe it to have fewer vulnerabilities, especially when used over an unsecure transport. I'd like to write a WAMP-SRP draft, but I cannot give any estimate as to when that will happen.

konsultaner commented 6 years ago

@ecorm I went through the RFCs to dig deeper into the topic. I found channel binding to be a quite useful feature, but I seem to not find a detailed description on how to implement it. You have a very detailed step by step description on how to generate hashes and messages for SCRAM. It would help people like me (that are not familiar with that topic) a lot if you could add what to use from the underlying TLS handshake in what step. Something like PlusHash(tlsMasterSecret) <- this is just an example and most probably wrong. At the moment I have no idea what to use where.

Is channel binding even possible in a browser? No, right?

The RFC for SCRAM has a sample for sha-1. It would be nice to have this for WAMP-SCRAM and its variants as well to have something to unit test on.

ecorm commented 6 years ago

@konsultaner I did not dig deeper into channel binding because it's not a feature we were planning to use for our product.

RFC5929 section 4.1 describes exactly how to hash the certificate data for the tls-server-end-point channel binding type. I don't think I can improve on the description in RFC5929, and I won't copy it verbatim in the WAMP-SCRAM spec. What I can do is provide a link/citation to that specific section for convenience.

AUTHENTICATE.Extra.cbind_data is where you put the Base64-encoded channel binding data. AUTHENTICATE.Extra.cbind_data is part of the the computed AuthMessage.

Note that the "SCRAM Algorithms" section is marked as non-normative, because the algorithms are already specified in the SCRAM RFC document.

Is channel binding even possible in a browser? No, right?

https://stackoverflow.com/questions/2402121/within-a-web-browser-is-it-possible-for-javascript-to-obtain-information-about

The RFC for SCRAM has a sample for sha-1. It would be nice to have this for WAMP-SCRAM and its variants as well to have something to unit test on.

RFC7677 has an example for PBKDF2+SHA256. I'll add a "Test Vectors" non-normative section that uses the example from RFC7677.

konsultaner commented 6 years ago

@ecorm Thank you!

konsultaner commented 6 years ago

@ecorm

  • The user associated with authid does not have permission to impersonate the given impersonated_id.
  • Ther server recognizes authid but does not recognize impersonated_id.

Shouldn't they be optional too and be handled like

  • (Optional) The server does not recognize the given authid.

because

  1. no one should be able to find out what an authids permissions are
  2. no one should find out if an authid passed in the impersonated_id is a valid authid
ecorm commented 6 years ago

@konsultaner

Shouldn't they be optional too and be handled like

When I wrote that, I though that for an unrecognized authid, the server wouldn't even bother checking bullet points 3 & 4.

But upon further thought, an attacker could very be a low-privilege user with a recognized authid that is trying to learn who the admin users are.

I'll make those two conditions optional. Good catch. :+1:

ecorm commented 6 years ago

@konsultaner Wait, now I remember why I did it that way.

The user associated with authid does not have permission to impersonate the given impersonated_id.

I think it's acceptable for a user to know if his/her own authid has permission to impersonate. It's not leaking any permission information about other users.

The server recognizes authid but does not recognize impersonated_id.

In that case, if the server ABORTs, nothing is leaked about the existence of impersonated_id. The attacker doesn't know if it's because he/she doesn't have permission to impersonate impersonated_id, or if it's because impersonated_id doesn't exist.

konsultaner commented 6 years ago

@ecorm

In that case, if the server ABORTs, nothing is leaked about the existence of impersonated_id. The attacker doesn't know if it's because he/she doesn't have permission to impersonate impersonated_id, or if it's because impersonated_id doesn't exist.

Lets say the router will send mock CHALLENGE messages for a wrong authids. If the attacker had an administrative authid (i.e. "root" in the worst case) that most probably is allowed to impersonate users, the attacker could brute force others. The attacker would use "root" as authId and whenever the router response is a CHALLENGE, the impersonated_id is a valid authid.

I know this scenario is not very likely to happen, but it could.

I think it's acceptable for a user to know if his/her own authid has permission to impersonate. It's not leaking any permission information about other users.

I'm not an security expert. Since the ABORT doesn't say why it was aborted, it is probably worth a discussion? I don't know.

oberstet commented 6 years ago

The impersonated_id has nothing to do with WAMP-SCRAM, so this should be removed from the proposal here ..

konsultaner commented 6 years ago

@oberstet this is a very useful feature and would fit very well into WAMP-SCRAM because it is security relevant as you can see in the discussion. Two of my clients need this feature and the use case is clearly defined. I would really like to see this coming with WAMP-SCRAM.

oberstet commented 6 years ago

@konsultaner as said, this has nothing to do with SCRAM, so it shouldn't be here. also, we just closed the actual issue;) https://github.com/wamp-proto/wamp-proto/issues/252 you might reopen that one of course ..

konsultaner commented 6 years ago

@oberstet #252 was session impersonation. But you are right. I'll open another issue, since impersonation may also work for WAMP-CRA, WAMP-SRP or other authentication methods too.

konsultaner commented 6 years ago

@oberstet actually:

SCRAM supports the SASL authorization identity defined in RFC4422, section 3.4.1. This SASL authorization identify allows a client to impersonate a user.

But still having it as a general feature would be nice to have. I'll open one anyway.

konsultaner commented 6 years ago

@ecorm You sometimes write Challange.Details.xxxx in your specs, but according to the WAMP-definition it should be Challange.Extra.xxxx

konsultaner commented 6 years ago

@ecorm, @oberstet Challenge.extra.iterations that is used in WAMP-CRA is renamed to Challenge.extra.cost in WAMP-SCRAM. Should we keep iterations, keep both or rename iterations in WAMP-CRA to cost?

konsultaner commented 6 years ago

@ecorm both the java implementation for Argon2 and BCrypt do not offer a version parameter. Are version_num and version_str really necessary? I just took a quick look into the libs, but it seems not necessary to have those version information.

ecorm commented 6 years ago

@oberstet

The impersonated_id has nothing to do with WAMP-SCRAM, so this should be removed from the proposal here.

impersonated_id corresponds to the authzid attribute that appears in several places in the SCRAM RFC. What WAMP refers to as authid, SCRAM uses the term username. If the authzid feature is used, SCRAM specifies that it should appear in the AuthMessage string that is used to compute the ClientProof and ServerSignature.

Having said that, SCRAM does not require the use of the authzid feature. If a user impersonation feature is accepted for WAMP in general, it could be incorporated into WAMP-SCRAM in the manner I originally proposed.

@konsultaner

You sometimes write Challange.Details.xxxx in your specs, but according to the WAMP-definition it should be Challange.Extra.xxxx

The CHALLENGE message section in the WAMP spec uses Extra, but the rest of the document, especially the Authentication section, uses CHALLENGE.Details. I adopted the convention that was already in use in the Authentication section.

the attacker could brute force others. The attacker would use "root" as authId and whenever the router response is a CHALLENGE, the impersonated_id is a valid authid.

Okay, you've convinced me about this possible attack vector. Thanks.

I'm not an security expert. Since the ABORT doesn't say why it was aborted, it is probably worth a discussion? I don't know.

Now that I think about it, an attacker could guess "admin" or "root" as the authid and learn that the admin account does have permission to impersonate users, which results in leaked security information.

BTW, WAMP-SCRAM allows the server to optionally return an error message via ABORT.Details. SCRAM defines a bunch of standard error strings that can be used for failed authentications.

I'll change the spec so that those two bullet points are (Optional).

konsultaner commented 6 years ago

The CHALLENGE message section in the WAMP spec uses Extra, but the rest of the document, especially the Authentication section, uses CHALLENGE.Details. I adopted the convention that was already in use in the Authentication section.

Is addressed here #97

ecorm commented 6 years ago

Challenge.extra.iterations that is used in WAMP-CRA is renamed to Challenge.extra.cost in WAMP-SCRAM. Should we keep iterations, keep both or rename iterations in WAMP-CRA to cost?

I used the more generic term "cost" in WAMP-SCRAM because bcrypt does not use a linear iteration count. The different authentication methods are going to need to generate different CHALLENGE.Details dictionaries anyway, so I don't see that as an obstacle for implementors.

both the java implementation for Argon2 and BCrypt do not offer a version parameter. Are version_num and version_str really necessary? I just took a quick look into the libs, but it seems not necessary to have those version information.

Argon2 has gone through several versions as the authors address vulnerabilities (it's up to v1.3 now). I assume that different versions can produce different hashes, so the version that was used to hash the password needs to be recorded in the password database and also passed to the client. bcrypt has also undergone a version change to fix a vulnerability.

In your case in Java, you can do something like this on the client:

if (authmethod == "wamp-scram-argon2")
{
    if (challenge.details.version_num != 0x13)
    {
        throw Error("Requested Argon2 version not supported on this client");
    }
    else
    {
        useArgon2WithoutVersionNumber(params...);
    }
}

If they release Argon2 v.1.4 in the future, I hope that your Java crypto library will still provide a v.1.3 implementation so that existing users can still log in before their password hashes are migrated to v.1.4.

You'll also find that some Argon2 implemetations (libsodium in particular) does not allow you to pass the parallelization parameter; they fixed it to p=1. scrypt on libsodium is even worse. They designed the hashing APIs this way because passwords are traditionally only hashed on the server once.

For Argon2, I'm considering fixing p=1 in order to open up more available crypto libraries. I think p is effectively the same as multiplier on the iteration count, but I need to research this some more.

KSDaemon commented 6 years ago

Hi all! Well, im not a big security guru... But why we can not keep .iterations or .cost or any other, related to used algorythm? I think Challenge.extra.* — is a good namespace for algorythm-specific options. But, again, may be i do not know all pros and cons.

ecorm commented 6 years ago

@KSDaemon Where the algorithm parameters are kept in the CHALLENGE.Details has no bearing on the security aspects of WAMP-SCRAM. (Note: CHALLENGE.Details and CHALLENGE.Extra are currently interchangeable in the WAMP spec).

Possibilities include:

oberstet commented 6 years ago

Instead of aiming to expose all possible knobs and whistles, I'd rather chose sensible defaults, with a minimal user configuration first, to see how that works .. all these possible knobs and combinations are what make TLS tricky in large parts. So I would rather avoid adding that to WAMP. What we need WAMP-SCRAM for fundamentally IMO is for closing a gap with authentication schemes that are vastly different. Eg this also applies to impersonate. We should hide that (not expose) for now, unless it is essential to SCRAM itself (some time ago I looked into that;) - WAMP authid is SCRAM username .. that's all we need for a basic integration it seems ..

oberstet commented 6 years ago

rgd WAMP-CRA renaming of attributes (eg iterations): no, I am -1 on that, as it is a breaking change, and it has no gain ..

ecorm commented 6 years ago

Instead of aiming to expose all possible knobs and whistles, I'd rather chose sensible defaults

If you're referring to hashing algorithm parameters, it's not feasible to "hard-code" them because they are intended to be re-tuned in the future as hardware performance advances. The exceptions to this might be the "p" parameter of Argon2 and the "r" parameter of scrypt.

WAMP router implementations can certainly choose to provide default algorithm parameters, but that is an implementation detail that is outside the scope of the WAMP spec, IMO.

I'd love for there to be only one "golden" hashing algorithm, but there is no agreement in the industry as to which is the best one. Some prefer older algorithms such as bcrypt or scrypt, because they have been battle-tested for longer. Others prefer bleeding edge such as Argon2. And others prefer the NIST-approved PBKDF2.

Even if WAMP-SCRAM covers four different hashing algorithms, nothing prevents a WAMP implementation from only implementing one or two among them. A WAMP-based application would likely only adopt one hashing algorithm.

Eg this also applies to impersonate. We should hide that (not expose) for now

Since user impersonation can done with other authmethods, I tend to agree that it should not be exposed in WAMP-SCRAM for now. I can lift all the impersonation stuff out of the WAMP spec and place it in a separate document for now, pending discussion/decision in issue #297.

I could do the same for the channel binding feature - lift it out of the "basic" WAMP-SCRAM spec.

rgd WAMP-CRA renaming of attributes: no, as it is a breaking change, and it has not gain ..

The suggestion was not to rename WAMP-CRA attributes, but to rename WAMP-SCRAM's cost attribute to be more in line with WAMP-CRA's iterations.

meejah commented 6 years ago

Regarding naming: I think it makes sense to keep familiar names for whatever standard (so, e.g. keep "cost" if that's the most-used term). I don't see any gain trying to make the attributes match (especially because they aren't the same -- just a similar concept).

@ecorm I think what @oberstet is getting at is: we should pick our favourite algorithm (for now?) to narrow the choices for users/implementors who don't want to learn about all of them and weigh the tradeoffs. We can always add more later. We can also choose to add algorithm support but still "bless" a best one (i.e. wording in the spec, like "if you don't know how to choose a hash algorithm, use X").

Similar for "cost" etc parameters: we probably should include them as knobs (because they're expected to get bigger), but we should definitely also include sensible defaults. Like "in 2018, cost parameter X is the default and is recommended [because ...]".

The point here, from my perspective, is: someone who knows little about hash algorithms (etc) should be able to turn on "WAMP-SCRAM" and still be provided with secure, sensible defaults and a working system. They definitely should NOT have to read a bunch of stuff and try to choose between pbkdf2, bcrypt, scrypt, argon2, etc etc. even if "advanced" users can go and do that. Any choices that are "definitely bad" should be removed (or not added).

I think we should choose "argon2" as the default, blessed password-hashing algorithm.

ecorm commented 6 years ago

@meejah

I think we should choose "argon2" as the default, blessed password-hashing algorithm.

At the very least, we should keep PBKDF2-SHA256, because that's the algorithm already specified in the standard SCRAM-SHA-256 (RFC7677), and some applications may be contractually bound to only using crypto functions approved by NIST. PBKDF2 is also implemented natively in the Javascript Web Crypto API (in secure contexts).

Don't forget that there are three "flavors" or Argon2: i, d, and id. I think id is the one generally recommended for password hashing - need to research that.

Perhaps PBKDF2 and Argon2id as the two possible choices, and we forget about bcrypt and scrypt? I personally wouldn't mind getting rid of bcrypt because of its password/key length restrictions. I'm also not fond of scrypt because it's difficult to tune the CPU cost independently of the memory cost.

Like "in 2018, cost parameter X is the default and is recommended [because ...]".

I'm not at all comfortable doing this within the WAMP spec, unless we can cite recommendations from authoritative sources. The general approach is to choose an acceptable run time (say 100ms), and tune the cost parameter on actual hardware until that run time is reached. This will be vastly different between mobile browsers and native desktop clients. The tunable memory parameter of Argon2 will also be vastly different among between hardware platforms.

RFC7677 recommends at least 4096 iterations of PBKDF2-SHA-256 as of November 2015, so we could simply quote that in the WAMP-SCRAM spec. Perhaps there's a similar recommendation in the Argon2 spec?

If the WAMP community wants to provide suggestions for parameters, perhaps it would be best done on a web page or "living document" outside of the WAMP specification (or even a StackOverflow community wiki). It would be even better if benchmark timing results were to accompany those recommendations.

Of course WAMP implementors would be free to provide whatever recommendations they want.

meejah commented 6 years ago

I'm not at all comfortable doing this within the WAMP spec, unless we can cite recommendations from authoritative sources.

Okay that's a fair point; if we can't find authoritative sources then I guess this should be dropped. Still, giving someone a bunch of parameters without recommendations sounds .. "bad".

From what I've read Argon2id basically rolls up all the same features as both bcrypt and scrypt combined -- leading to a conclusion of "keep PBKDF2 for 'older/wider-support' use-cases and use argon2id as the recommended approach". If we wanted to keep one of bcrypt or scrypt I'd also rather keep scrypt (but also happy to drop both in favour of argon2id)

I see "the WAMP specification" as a "living document" -- couldn't the WAMP community recommend upgrades/changes to the recommendations via pull-requests (for example)? But perhaps you're right that the spec should just leave it up to implementors to make recommendations for their users, as they see fit?

ecorm commented 6 years ago

I see "the WAMP specification" as a "living document" -- couldn't the WAMP community recommend upgrades/changes to the recommendations via pull-requests (for example)?

It's currently a living document now while it's hosted on GitHub, but that will no longer be the case if/when it becomes an RFC. I picture GitHub as the "develop branch" for the WAMP spec, while the RFC is going to be a "master branch" release.

meejah commented 6 years ago

@ecorm re "living doc": okay I see what you mean. I would still be in favour of putting recommendations in the spec (with dates and assumptions stated). Ideally these could just be references to already-published recommendations.

For comparison: the scrypt paper includes particular parameters (with a date, and the processor used) but the RFC for scrypt does not.

The argon2 RFC draft includes a rule-based method of selecting the parameters, and includes example parameters for different use-cases (with processor speeds and memory specified).

ecorm commented 6 years ago

Folks, can I get a thumbs up or down on this comment about restricting the WAMP-SCRAM hashing algorithms to only PBKDF2 and Argon2? The rationale for this restriction is to not overwhelm WAMP users/implementors with four different possible hashing algorithms. If thumbs down, please explain why.

Reason for PBKDF2: Approved by NIST and is a well known mature algorithm. Native implementation available in the Javascript Web Crypto API in secure contexts (i.e. HTTPS websites).

Reason for Argon2: Winner of the Password Hashing Competition and positioned to be the successor to scrypt and bcrypt. It's the default password hashing algorithm in libsodium and is now also supported in PHP. Used by KeePass.

konsultaner commented 6 years ago

"keep PBKDF2 for 'older/wider-support' use-cases and use argon2id as the recommended approach"

+1 for PBKDF2-SHA256, because implementers have used it for WAMP-CRA and are familiar with it +1 for one extra KDF, if Adrgon2id is the best for the WAMP-SCRAM +1 for that

It was not hard to find libs to implement all suggested KDFs, but having no idea how to choose the best one, makes me feel like having to study all of them. Implementers shouldn't have to do it.

rgd WAMP-CRA renaming of attributes (eg iterations): no, I am -1 on that, as it is a breaking change, and it has no gain ..

@oberstet I'm totally with you that renaming is not a good idea. Whats with extra/details in CHALLENGE? Shouldn't that be fixed first? (#97)

meejah commented 6 years ago

Going through the spec more closely, some notes/questions/comments:

ecorm commented 6 years ago

@meejah Thanks for the detailed critique! :+1:

is there reasoning behind making the variants their "own" auth-method (e.g. "wamp-scram-pbkdf2") versus having one auth-method ("wamp-scram") with an option ("algorithm=pbkdf2" or similar)?

There's already a mechanism for the client announcing its supported authmethods in the HELLO message. I felt it would have been more complicated to have two "levels" of authmethod announcement (one level for wamp-scram, and another level for algorithms), so I essentially flattened it into one level.

Let's say a client announces "authmethods": ["wamp-scram", "wamp-srp"], and both of those authmethods have their own variants. How can the client announce its supported authmethod variants in HELLO.Details without there being an attribute name collision? "wamp-scram-algorithms": ["argon2", "pbkdf2"] and "wamp-srp-algorithms": ["argon2", "pbkdf2"]? How do we manage the HELLO.Details "namespace" for all possible authmethods? Seems complicated to me.

One could ask if the client should even bother announcing its supported wamp-scram variants? If not, is it acceptable for the client to ABORT the authentication exchange once it realizes that it doesn't support the algorithm requested by the server for that particular user?

I we want to place the burden on the server to check that the client supports the required algorithm+version, then I think the authmethod string should be something like wamp-scram-argon2id-13.

the version_num versus version_str stuff feels odd; could this instead be rolled into an "algorithm" string too? So like "argon2id-13" (for "argon2id" version "1.3") or "bcrypt-2b" (for "bcrypt" version "$2b$")?

I wanted to avoid a single version attribute that could be either a string or a number, which is painful to deal with in statically typed languages.

I've seen an Argon2 API (libsodium, I believe) that takes a version argument as an integer. I thought that having the version in its own attribute would alleviate the burden of parsing the version number out of a more complex string.

I now think that the version suffix should be part of the authmethod string.

for nonces, would it simplify implementations if the spec decided "it's a base64-encoded number" instead of just an arbitrary character sequence?

Yes, I think it would simplify things, since base64 output is guaranteed not to contain the illegal , character. Implementations are already going to need base64 for other parts of the algorithm anyway.

gs2_cbind_flag doesn't exactly roll off the tongue; is there a reason for that name?

I almost went with the more readable channel_binding, but then changed my mind to match the terminology used by the SCRAM/SASL RFCs. After seeing your critique, I feel that I want to go back to channel_binding. I now see that Crossbar uses channel_binding for its cryptosign authmethod, so perhaps it's better that WAMP-SCRAM adopts the same attribute name for consistency.

in the table for mapping SCRAM->WAMP messages, the upper-right corner should read "HELLO" (not "WELCOME") I think?

Yes, that's correct. Good catch!

there's already "authextra" in HELLO -- does it make sense to put the "nonce", "gs2_cbind_flag" and "impersonation_id" into that instead of additional "top level" HELLO keys? (Or am I just getting confused by implementation details in Autobahn-Python?)

None of the authmethods that are currently in the WAMP spec make use of authextra (authextra is not found anywhere in the spec). But I now see that Crossbar/Autobahn uses it for its "not-yet-standard" authmethods.

I agree that it would be better for WAMP-SCRAM to follow the same convention for using authextra.

oberstet commented 6 years ago

IMO, in general, this is exposing way to many knobs, and letting client-server negotiate details is also an invitation for trouble. Also, @ecorm could you pls file a PR so we can look at the proposed text?

gs2_cbind_flag

If that is supposed to work like for WAMP-cryptosign, then this isn't flag, and the attribute should be name channel_binding

impersonation_id

As mentioned, I am -1 on exposing this

ecorm commented 6 years ago

@oberstet

IMO, in general, this is exposing way to many knobs

If you're referring to the number of possible algorithms, there seems to be consensus here to reducing the set of algorithms to only PBKDF2 and Argon2id. I've described the rationale for those two in https://github.com/wamp-proto/wamp-proto/issues/135#issuecomment-370584383. We could recommend Argon2id as the default, with PBKDF2 also available for those applications that are restricted to NIST-approved crypto functions. Most crypto libraries providing SHA-256 and HMAC also provide PBKDF2, so it wouldn't be that much more work to provide PBKDF2 in addition to Argon2.

As for the version value, this is required to be able to cope with future changes to Argon2 which results in different hashes for the same inputs. If a hypothetical Argon2 v1.4 is released someday to fix a vulnerability, applications will need to be able to continue authenticating against older v1.3 hashes until they can be upgraded to v1.4 hashes.

The other "knobs", cost and memory, need to be tuned for the client platform that will perform the hashing. If we fix those values to the "least common denominator", which would be a mobile web browser using a Javascript crypto library, then we are penalizing the native desktop clients that are able to compute much stronger hashes.

We can take a cue from the libsodium library, and hide the parallel knob and fix it to 1. That would be one less thing for the application developer to worry about. Other than that, I'm sorry, but I don't know what else I can do remove some of the knobs.

and letting client-server negotiate details is also an invitation for trouble.

There is no negotiation involved. Once a password has been hashed using a certain algorithm/version, the client MUST support that same algorithm/version to authenticate the user.

If your concern is with the algorithm and version number being appended to the wamp-scram authmethod string, I can understand how that might be tricky for routers to handle the different combinations when interpreting the HELLO message. I'm starting to think that @meejah's suggestion to have a separate attribute like "algorithm": "argon2id13" might be best.

Also, @ecorm could you pls file a PR so we can look at the proposed text?

I'll do so for the next iteration of my proposal. I wanted to get some initial feedback before submitting a PR. The current proposed text is here.

If that is supposed to work like for WAMP-cryptosign, then this isn't flag

I don't know about WAMP-cryptosign, but I can assure you that the SCRAM RFC calls it "channel binding flag". I need to investigate how WAMP-cryptosign uses its channel_binding parameter to make sure there is no confusion.

impersonation_id As mentioned, I am -1 on exposing this

I already said that I'd take it out of my proposal, pending a resolution to #297.

meejah commented 6 years ago

For the "algorithm" vs. auth-method name things, I think we should do this:

Thus: there is no negotiation, the authmethod string is straightforward and it's more-obvious what to choose.

meejah commented 6 years ago

As for the other knobs, the situation is:

It's tempting to try and get rid of the argon2 params (or roll them up; e.g. "memory is always cost / 4 and parallel is always 2"). But I guess also if you were just forced to learn about "cost" and PBKDF2 vs. Argon2 then you might be surprised that the extra Argon parameters aren't there for some reason :)

meejah commented 6 years ago

Probably worth noting that in any case it's only "the person running Crossbar" that has to think about the parameters, and they could have "sensible defaults" (decided by the router authors).

meejah commented 6 years ago

Hmm, argon2_cffi outputs v=19 currently -- so the algorithm should maybe be argon2id-19?

ecorm commented 6 years ago

@meejah

the authmethod is "wamp-scram"

I agree

there is an "algorithm" (or "scram_hash_algorithm" or similar) field saying which algorithm to use (if unspecified, argon2id-13 is used)

I agree in principal, but prefer to name it the "kdf" field, as it's only one step in the overall algorithm. I agree that the version information should be appended.

all implementations MUST support both algorithms. Thus: there is no negotiation, the authmethod string is straightforward and it's more-obvious what to choose.

I agree about both being mandatory to avoid the need to "negotiate". I'm not comfortable with legacy-pbkdf2 instead of pbkdf2 but what we can do is write non-normative text in the spec that explains why Argon2 might be preferable, and that PBKDF is also supported both those applications that are restricted to algorithms approved by standards bodies.

cost (or something similar) is definitely needed (for both algorithms) a salt is required (for both algorithms) memory and parallel are only parameters for argon2id

Now that bcrypt is out of the picture, we can go back to naming the CPU cost iterations. "iterations" is the term used by both the PBKDF2 and Argon2 literature.

It's tempting to try and get rid of the argon2 params (or roll them up; e.g. "memory is always cost / 4 and parallel is always 2"). But I guess also if you were just forced to learn about "cost" and PBKDF2 vs. Argon2 then you might be surprised that the extra Argon parameters aren't there for some reason :)

I'm going to research why libsodium locks the parallelization factor to 1. libsodium is a widely-used crypto library, with bindings to several languages, and it would be a shame to force WAMP-SCRAM implementors to not be able to use it.

Hmm, argon2_cffi outputs v=19 currently -- so the algorithm should maybe be argon2id-19?

19 is the decimal representation of hexadecimal 0x13. 1.3 is the actual, current version number.

ecorm commented 6 years ago

Okay, I've figured out Argon2's parallelization parameter. On crypto libraries that support multithreading, this will make the library use multiple threads to compute the hash. With iterations and memory being equal, if two cores are used to compute a hash (with p=2), the resulting hash will be stronger (and therefore harder to crack) than a hash with p=1 computed in the same amount of time.

The p parameter can thus be useful to make a multi-cored server compute a stronger hash in a given amount of time. However, for WAMP-SCRAM, the hash has to be computed client-side, and I don't think an application developer should be making assumptions about the availability of multiple cores on their client platforms.

On crypto libraries that don't support multi-threading, if the p parameter is greater than 1, the library will have to perform the parallel work sequentially. On such mono-threaded libraries, the p parameter effectively becomes a multiplier on iterations (and possibly a divider on memory). I think this is why the libsodium library locks the p parameter to 1.

Given that WAMP-SCRAM requires the password hash to be computed by the client, I think it's reasonable to restrict the Argon2 parallellization parameter to 1 for the WAMP-SCRAM authentication method. We can explain this rationale in the spec so that security-savvy folks aren't scratching their heads about why the parallelization parameter is not tunable.