Feat[MQB]: add authentication with basic logic #746
Conversation
emelialei88
force-pushed
the
feat-authn-protocol-mqb
branch
from
f71824d to
aa65195
Compare
| const AuthenticationContextSp& context) | ||
| { | ||
| // PRECONDITIONS | ||
| BSLS_ASSERT_SAFE( |
There was a problem hiding this comment.
Fine to have preconditions, but they aren't preconditions if they're not documented in the function contract: The behavior of this function is undefined unless .... Otherwise, how would a caller of this function know it will crash if !context->d_initialConnectionContext_p->isIncoming() && !context->d_isReversed? We'd be lying to them.
Second concern: are these preconditions checked or enforced by the caller? I think they are, but the second one below has me worried. Is there any way we could get a bad message over the wire from a client and crash?
There was a problem hiding this comment.
I'm not sure I understand the second concern.
isIncoming is set by us when TcpSessionFactory receives an incoming call (true) or sending an outbound message (false).
For negotiation, (and it should be the same for authentication),d_isReversed is default to be false unless we receive an reversed connection request (type bmqp_ctrlmsg::NegotiationMessage ::SELECTION_INDEX_REVERSE_CONNECTION_REQUEST) or we're about to send out a reverse connection request.
| bmqp_ctrlmsg::AuthenticateRequest& authenticateRequest = | ||
| context->d_authenticationMessage.authenticateRequest(); | ||
|
|
||
| BALL_LOG_DEBUG |
There was a problem hiding this comment.
I am a little concerned about logging arbitrary data that we get from the network, but I think the operator<< for authenticateRequest as generated is fine with this. This is another place where base64 may help compress the logs.
There was a problem hiding this comment.
I could get rid of printing the data, but sometime maybe we just want to debug and see what it is. Is setting it to debug mode still not okay?
| else if (event.isControlEvent()) { | ||
| const int rc = event.loadControlEvent(&negotiationMessage); | ||
| if (rc != 0) { | ||
| BALL_LOG_ERROR << "Invalid response from broker [reason: 'control " |
There was a problem hiding this comment.
Is this a response from broker (used to be negotiation message)?
And we do need to log/dump the blob
There was a problem hiding this comment.
And we do need to log/dump the blob
Is this a question? Do you mean we need to keep the error log of event or I should log even there's not an error?
There was a problem hiding this comment.
And we do need to log/dump the blob
Is this a question? Do you mean we need to keep the error log of event or I should log even there's not an error?
I think, it would be helpful; to log/dump the blob when there is an error (rc != 0). Seems like we had this in the previous version of the code
emelialei88
force-pushed
the
feat-authn-protocol-mqb
branch
9 times, most recently
from
e1fe5b9 to
d8123e8
Compare
emelialei88
force-pushed
the
feat-authn-protocol-mqb
branch
2 times, most recently
from
501f217 to
95f9480
Compare
emelialei88
force-pushed
the
feat-authn-protocol-mqb
branch
3 times, most recently
from
ce8e150 to
fa1fdd7
Compare
Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
…ation Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
emelialei88
force-pushed
the
feat-authn-protocol-mqb
branch
from
fa1fdd7 to
d364750
Compare
Signed-off-by: Emelia Lei <[email protected]>
emelialei88
force-pushed
the
feat-authn-protocol-mqb
branch
from
d364750 to
a33efc3
Compare
Handing authentication logic on the broker side:
AuthenticationEventevent type.AuthenticationContextto encapsulate relevant context data.InitialConnectionHandlerto handle the authentication flow.At this stage, we only handle incoming authentication requests and always allow them to succeed. Outbound, reverse authentication requests initiated by a broker, and re-authentication are not supported.