DRIVERS-1873 Use OP_MSG to authenticate if the server supports OP_MSG #1116
Conversation
| @@ -453,13 +453,16 @@ Establishing a Connection (Internal Implementation) | |||
|
|
|||
| Before a `Connection <#connection>`_ can be marked as either "available" or "in use", it | |||
| must be established. This process involves performing the initial | |||
| handshake, handling OP_COMPRESSED, and performing authentication. | |||
| handshake, inspecting the value of ``maxWireVersion`` (to decide whether to | |||
| use ``OP_MSG``(>=3.6) or ``OP_QUERY``(<3.6) for future communications), | |||
There was a problem hiding this comment.
Here and in the change in OP_MSG you should say what the relevant wire version is.
source/message/OP_MSG.rst
Outdated
| inspected the value of ``maxWireVersion``, it must use either ``OP_MSG`` | ||
| (``maxWireVersion >= 6``) or ``OP_QUERY``(``maxWireVersion < 6``) | ||
| respectively. Starting in MongoDB 6.0, using ``OP_QUERY`` for anything other | ||
| than the MongoDB handshake will result in an error. |
There was a problem hiding this comment.
This seems to be restating this existing rule:
If the node supports OP_MSG, any and all messages MUST use OP_MSG, optionally compressed with OP_COMPRESSED.
What do you think about mentioning the auth commands specifically in that line instead?
| because starting with MongoDB 6.0 using ``OP_QUERY`` for anything other than | ||
| the handshake will result in an error. Once the driver has performed the | ||
| handshake and inspected the value of ``maxWireVersion`` it MUST use | ||
| ``OP_MSG`` if supported. |
There was a problem hiding this comment.
Should we link to the section in the OP_MSG spec instead of duplicating it? Alternatively, I would be more open to duplicating here once we make it more succinct (following my comment in OP_MSG). Or could we just remove this section altogether?
There was a problem hiding this comment.
I reduced it down significantly because the pseudocode offers some of the details
|
|
||
| .. code:: | ||
|
|
||
| try: | ||
| connect connection via TCP / TLS | ||
| perform connection handshake | ||
| inspect "maxWireVersion" |
There was a problem hiding this comment.
I propose we clarify the pseudocode like this rather than just "inspect maxWireVersion":
handle OP_COMPRESSED
if maxWireVersion >= 6: # MongoDB 3.6+
perform connection authentication via OP_MSG
else:
perform connection authentication via OP_QUERY
|
I think this requirement is better suited for the auth spec rather than CMAP, since CMAP is more about managing the connections rather than any connection-related implementation details. I think the section about the auth handshake seems like a good option. |
juliusgeo
requested review from
JamesKovacs and
ShaneHarvey
and removed request for
a team
| #. Inspect the value of ``maxWireVersion``. If the value is greater than or | ||
| equal to ``6``, then the driver MUST use ``OP_MSG`` for authentication. | ||
| Otherwise, it MUST use ``OP_QUERY``. | ||
|
|
There was a problem hiding this comment.
Could you update the "Spec Version" to 1.10.5, bump the "last modified date", and add a changelog entry at the bottom?
source/auth/auth.rst
Outdated
| @@ -1262,6 +1266,10 @@ Q: Should drivers support accessing Amazon EC2 instance metadata in Amazon ECS? | |||
| Version History | |||
| =============== | |||
|
|
|||
| Version 1.10.5 Changes | |||
| * Clarify that``OP_MSG`` must be used for authentication when it is | |||
There was a problem hiding this comment.
Missing space between "that" and OP_MSG.
There was a problem hiding this comment.
Oops. Sorry about that. Fixed.
source/message/OP_MSG.rst
Outdated
| @@ -60,7 +60,8 @@ node supports ``OP_MSG``. Drivers that have only ever supported MongoDB 3.6 and | |||
| newer MAY default to using ``OP_MSG``. | |||
|
|
|||
| If the node supports ``OP_MSG``, any and all messages MUST use ``OP_MSG``, | |||
| optionally compressed with ``OP_COMPRESSED``. | |||
| optionally compressed with ``OP_COMPRESSED``. This includes authentication | |||
| messages. | |||
There was a problem hiding this comment.
This is ambiguous. One possible reading is that authentication messages must use OP_MSG (which is true) and optionally be compressed with OP_COMPRESSED (which is false). Authentication-related messages MUST NEVER be compressed. They were explicitly excluded from compression because of concerns about unintentionally introducing security issues. The following is a list of commands that must never be compressed used by the .NET/C# driver:
hello
isMaster
saslStart
saslContinue
getnonce
authenticate
createUser
updateUser
copydbsaslstart
copydbgetnonce
copydb
If we want to reconsider compressing these messages we can do that, but it seems outside the scope of this change.
There was a problem hiding this comment.
That makes sense. Fixed.
source/message/OP_MSG.rst
Outdated
| @@ -60,7 +60,8 @@ node supports ``OP_MSG``. Drivers that have only ever supported MongoDB 3.6 and | |||
| newer MAY default to using ``OP_MSG``. | |||
|
|
|||
| If the node supports ``OP_MSG``, any and all messages MUST use ``OP_MSG``, | |||
| optionally compressed with ``OP_COMPRESSED``. | |||
| optionally compressed with ``OP_COMPRESSED``. Authentication messages MUST | |||
| also use ``OP_MSG`` when it is supported, but MUST not use ``OP_COMPRESSED``. | |||
There was a problem hiding this comment.
"MUST not" -> "MUST NOT"
See:
The keywords "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119.
No description provided.