- 
                Notifications
    
You must be signed in to change notification settings  - Fork 26
 
Added RSA encryption #327
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
base: trunk
Are you sure you want to change the base?
Added RSA encryption #327
Conversation
8be6b8e    to
    a2dcff0      
    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.
Thanks for the PR. this is major feature 👍 Overall LGTM. I’ve left a few comments. If feasible, please add a minimal integration test
        
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlConnectionFactoryProvider.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                r2dbc-mysql/src/test/java/io/asyncer/r2dbc/mysql/RsaEncryptionTest.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlConnectionFactoryProvider.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlConnectionConfiguration.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/authentication/MySqlAuthProvider.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlConnectionConfiguration.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/authentication/MySqlAuthProvider.java
              
                Dismissed
          
            Show dismissed
            Hide dismissed
        
      Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
8d39e66    to
    0ded89c      
    Compare
  
    | 
           @jchrys I've made those changes you suggested. I was unable to figure out how to configure the integration tests and get them working with the test container so I've removed them. It's not ideal but if it's completely necessary then I can try to work on the tests in the future when I have time but otherwise I think it's ready.  | 
    
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.
I did some testing and debugging—please take a look at my comments and let me know once you’ve fixed them so I can review again.
        
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlConnectionFactoryProvider.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
        
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/authentication/MySqlAuthProvider.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/authentication/AuthUtils.java
          
            Show resolved
            Hide resolved
        
      Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
| 
           @jchrys Ready for review again with changes.  | 
    
Signed-off-by: S V <[email protected]>
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.
LGTM overall. I’ll run comprehensive test next week and post comment then; feel free to keep refining meanwhile.
        
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/MySqlConnectionConfiguration.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                r2dbc-mysql/src/main/java/io/asyncer/r2dbc/mysql/authentication/MySqlAuthProvider.java
          
            Show resolved
            Hide resolved
        
      Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
Signed-off-by: S V <[email protected]>
| 
           @svats0001 Great work! Thanks a lot.  | 
    
#326 @jchrys
Motivation:
To use RSA encryption when using the caching_sha2_password authentication type when SSLMode is disabled instead of throwing R2dbcPermissionDeniedException.
Modification:
MySqlAuthProvider - added RSA encryption method that returns encrypted bytes.
InitFlow - added serverRSAPublicKeyFile field to HandshakeExchangeable and invoked RSA encryption method after AuthUtils.authentication if SSL not completed and SSL necessary and serverRSAPublicKeyFile not null.
MySqlConnectionConfiguration - Added nullable serverRSAPublicKeyFile field.
MySqlConnectionFactory - Added serverRSAPublicKeyFile field to relevant constructor invocations inside getMySqlConnection.
MySqlConnectionFactoryProvider - Added option for serverRSAPublicKeyFile.
MySqlConnectionFactoryProviderTest - Added unit test to check if MySqlConnectionFactory is returned from ConnectionFactoryOptions containing serverRSAPublicKeyFile.
SslTunnelIntegrationTest - Added unit test to check if correct exceptions are thrown or if MySqlConnectionFactory is returned depending on various combinations of SSL and serverRSAPublicKeyFile in MySqlConnectionConfiguration and getting MySqlConnectionFactory from it.
Result:
When a serverRSAPublicKeyFile is provided and SSL is disabled and the caching_sha2_password authentication type is used by the MySql server and you're in full authentication phase, a R2dbcPermissionDeniedException will not be thrown and instead the password will be encrypted with RSA. Drawbacks are that RSA encryption method introduces many additional possible exceptions if specified file path is invalid or public key can't be read properly etc.
EDIT: Will fix checks/test on Monday