- 
                Notifications
    
You must be signed in to change notification settings  - Fork 155
 
Feat[MQB]: Make authentication workflow use plugins #773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat[MQB]: Make authentication workflow use plugins #773
Conversation
d8e0447    to
    0f8a365      
    Compare
  
    057a7c2    to
    ec64999      
    Compare
  
    d74e1fe    to
    2d81dff      
    Compare
  
    85e7ca0    to
    6441933      
    Compare
  
    a6885dd    to
    c2820a5      
    Compare
  
    6441933    to
    dae302b      
    Compare
  
    c2820a5    to
    1217636      
    Compare
  
    9f73e62    to
    3508355      
    Compare
  
    09f7274    to
    57a2d8f      
    Compare
  
    57a2d8f    to
    57e5b06      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial review
| STOP_OBJ(d_configProvider_mp, "ConfigProvider"); | ||
| STOP_OBJ(d_statController_mp, "StatController"); | ||
| STOP_OBJ(d_authenticationController_mp, "AuthenticationController"); | ||
| STOP_OBJ(d_statController_mp, "StatController"); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the DESTROY_OBJ change below look important.  Is this because we use stats in our auth controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. This change is made since StatController starts first before AuthenticationController, so when stopping I wanted to have them in reverse order.
a873f88    to
    fbb6bec      
    Compare
  
    1217636    to
    1128950      
    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]>
Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
Signed-off-by: Emelia Lei <[email protected]>
6f49b6b    to
    da77f6c      
    Compare
  
    Signed-off-by: Emelia Lei <[email protected]>
da77f6c    to
    524583f      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to revisit the reason for shared_from, see my big comment
Few other comments.
| 
               | 
          ||
| int rc = rc_SUCCESS; | ||
| bsl::string error; | ||
| mqbnet::InitialConnectionEvent::Enum input; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| mqbnet::InitialConnectionEvent::Enum input; | |
| mqbnet::InitialConnectionEvent::Enum event = InitialConnectionEvent::e_ERROR; | 
And remove all input = InitialConnectionEvent::e_ERROR;
| if (processRc != rc_SUCCESS) { | ||
| rc = (processRc * 10) + rc_PROCESS_AUTHENTICATION_FAILED; | ||
| error = processErrStream.str(); | ||
| input = InitialConnectionEvent::e_ERROR; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| input = InitialConnectionEvent::e_ERROR; | 
| 
               | 
          ||
| // In the case of a default authentication, we do not need to send | ||
| // an AuthenticationResponse, we just need to continue the negotiation. | ||
| if (context->state() == | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safer way is to explicitly specify the e_DEFAULT_AUTHENTICATING case as an argument to Authenticator::handleAuthentication and not read the state which the FSM can potentially change.
Or, introduce Authenticator::handleDefaultAuthentication.
Or, make a decision on something other than the state, something immutable.  For example, authenticationMsg.
Notice how InitialConnectionContext calls d_authenticator_p->anonymousCredential(), can it be done by the authenticator itself?
| /// context. | ||
| /// It is set during the initial authentication, and is null for | ||
| /// reauthentication. | ||
| InitialConnectionContext* d_initialConnectionContext_p; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we do not need it!
| << channel->peerUri() << "' [error: " << errorName | ||
| << ", code: " << errorCode << "]"; | ||
| 
               | 
          ||
| bmqio::Status status(bmqio::StatusCategory::e_GENERIC_ERROR, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to use less generic error category (e_CANCELED?)
| void InitialConnectionContext::readCallback(const bmqio::Status& status, | ||
| int* numNeeded, | ||
| bdlbb::Blob* blob) | ||
| { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// executed by one of the *IO* threads
| bmqu::MemOutStream errStream; | ||
| int rc = rc_SUCCESS; | ||
| 
               | 
          ||
| rc = readBlob(errStream, &outPacket, &isFullBlob, status, numNeeded, blob); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor. why not check the status before calling readBlob?
| 
               | 
          ||
| bsl::shared_ptr<mqbnet::Session> session; | ||
| 
               | 
          ||
| if (rc == 0 && d_state == State::e_NEGOTIATED) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if using enum rc_SUCCESS, let's use it everywhere
| bsl::shared_ptr<InitialConnectionContext> self = shared_from_this(); | ||
| 
               | 
          ||
| bmqu::MemOutStream errStream(d_allocator_p); | ||
| int rc = rc_SUCCESS; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly already made suggestion to initialize rc with rc_ERROR.
Easier to explicitly set rc_SUCCESS than making sure we don't handle error without rc = rc_ERROR;
| const bsl::variant<bsl::monostate, | ||
| bmqp_ctrlmsg::AuthenticationMessage, | ||
| bmqp_ctrlmsg::NegotiationMessage>& message) | ||
| { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// executed by *ANY* thread
71f8e49    to
    05f71f1      
    Compare
  
    Signed-off-by: Emelia Lei <[email protected]>
05f71f1    to
    899a9ea      
    Compare
  
    Signed-off-by: Emelia Lei <[email protected]>
d49eb88
      into
      
  
    bloomberg:integration/authn-authz
  
    | 
           
  | 
    
This PR apply plugin into authentication and reauthentication when we as a broker receives authentication requests from clients.
rawclientto support integration testtest_authn.py.bmqauthnbasicfor testing. It uses "basic" as mechanism and creates an in-memory map for username and password.Authenticatoris managed byTransportManagerand referred to byInitialConnectionContext.AuthenticationRequest.channelis closed when there's a failure.AuthenticationContextis created during initial connection, create areauthenticateCbto be called when a client session later receives anAuthenticationEvent.AuthenticationContext::Stateto indicate the state of authentication. This is used as to make sure only one authentication is run for a given context, and when a channel closes, no more reauthentication timer will be scheduled.ClientIdentityprior to anyAuthenticateRequest, we use default credential to authenticate, unless it's explicitly set toDisallow.AnonyPassAuthenticator(implementingmqbplug::Authenticator) and the corresponding factory and result undermqbauthnas defaultAuthenticator.TransportManagerinstead ofInitialConnectionHandlerown theNegotiator.InitialConnectionContext::Stateto indicate the state of initial connection.