[ http://issues.apache.org/jira/browse/DERBY-528?page=comments#action_12423493 ] Sunitha Kambhampati commented on DERBY-528: -------------------------------------------
Thanks Francois for working on this feature. I think this will be a great addition to Derby. The v3 patch does not apply cleanly in my eclipse IDE but it was fairly easy to take care of the conflict. Maybe it might be best to regenerate a patch. Thanks for taking care of most of the unwanted diffs, but there are still places where there are spurious diffs because of tabs/spaces etc. It might be good to remove them. TESTING: Since the codepath for the builtin authentication is changed when security mechanism is set to 8, I think some tests should be added when builtin authentication is used, and security mechanism is set to 8. First, just some thoughts. I think they are fine, but just wanted to mention them in case someone else had a better idea for these. a) java/engine/org/apache/derby/iapi/reference/Attribute.java I am not sure if this class should have the internal variables like drdaSecTokenIn etc. I remember Dan opening a jira issue http://issues.apache.org/jira/browse/DERBY-1151 mentioning about how instance variables in Attribute.java are picked up by ij. b)java/drda/org/apache/derby/impl/drda/DRDAConnThread.java + private final static String AUTHENTICATION_PROVIDER_BUILTIN_CLASS = + "org.apache.derby.impl.jdbc.authentication.BasicAuthenticationServiceImpl"; + + private final static String AUTHENTICATION_PROVIDER_NONE_CLASS = + "org.apache.derby.impl.jdbc.authentication.NoneAuthenticationServiceImpl"; + Wonder if it is ok to use the implementation class names here. Is it possible to let the authentication to just fail, if the authentication is not builtin and if it is not none. I think these checks are made for the handshake between client and server, in order to support the upgrade of the security mechanism, correct? c) Index: java/client/org/apache/derby/client/net/NetConnection.java I am not sure I understand these checks. Will there be a case when the dataSource_.getUser() will not be the same as the dataSource_.propertyDefault_user. Can you elaborate on this case. and why do we not update the user_ ? + String userName = user_; + ..... + if (dataSource_ != null) + { + String dataSourceUserName = dataSource_.getUser(); + if (!dataSourceUserName.equals("") && + userName.equalsIgnoreCase( + dataSource_.propertyDefault_user) && + !dataSourceUserName.equalsIgnoreCase( + dataSource_.propertyDefault_user)) + { + userName = dataSourceUserName; + } + } Some issues: 1)Non portable code. String.getBytes() will use the default encoding. This code wont work if the client and server are on different platforms with different default encoding. In Index: java/client/org/apache/derby/client/am/EncryptionManager.java + byte[] userBytes = userName.getBytes(); Also in code added in BasicAuthenticationServiceImpl.java Maybe you can use the toHexByte() to get bytes from the string. http://wiki.apache.org/db-derby/AvoidNonPortableMethods 2)Index: java/client/org/apache/derby/jdbc/ClientBaseDataSource.java Can you please explain why the below three lines have been removed. @@ -887,15 +904,8 @@ if (prop.containsKey(Attribute.CLIENT_TRACE_APPEND)) { setTraceFileAppend(getTraceFileAppend(prop)); } - if (prop.containsKey(Attribute.CLIENT_SECURITY_MECHANISM)) { - setSecurityMechanism(getSecurityMechanism(prop)); - } if (prop.containsKey(Attribute.CLIENT_RETIEVE_MESSAGE_TEXT)) { setRetrieveMessageText(getRetrieveMessageText(prop)); } } 3) Index: java/client/org/apache/derby/client/am/EncryptionManager.java I think this has to be within the SanityManager.DEBUG block. + // Assert we have a SHA-1 Message Digest already instantiated + SanityManager.ASSERT((messageDigest != null) && + (SHA_1_DIGEST_ALGORITHM.equals( + messageDigest.getAlgorithm()))); trace calls in DRDAConnThread should be in SanityManager.DEBUG blocks. + trace("parseACCSEC - SECCHKCD_NOTSUPPORTED [1] - " + securityMechanism + " <> " + server.getSecurityMechanism() + "\n"); Rest are minor comments - might be good to add more comments if possible. 4)Might be good to add javadoc like comments for the arguments also, specially in case of this method. I like the comments you added, may be good to explicitly mention that the sourceSeed is Rdr and the targetSeed_ is Rds. EncryptionManager.java + */ + public byte[] substitutePassword( + String userName, + String password, + byte[] sourceSeed_, + byte[] targetSeed_) throws SqlException { In Attribute.java: It would be nice to have a comment, just a line that says this is RDr or the seed sent by the client. Attribute.DRDA_SECTKN_IN In DRDAConnThread.validateSecMecUSRSSBPWD() + if (database != null) + database.secTokenOut = myTargetSeed; I think, if we reached to this point, database wont be null. If database is null, I think there will be some other serious issue. ClientBaseDataSource.java. I think the comment needs to be updated. + * 3. if password is available, if client does not support EUSRIDPWD, then + * USRSSBPWD is returned. -- Most of the toHexByte, toHexString functions seem to be copied across multiple places. I did see that you added comments that all these functions should probably be shared. Maybe it might be best to add a jira issue(?), so when we get back to discussing code sharing, we will clean up all this code that seems to be now added in two places apart from existing engine code. -- Comments in testSecMec.java should probably be updated to reflect that upgrade to USRSSBPWD does not happen currently. Thanks so much for all your work on this. Sunitha. > Support for DRDA Strong User ID and Password Substitute Authentication > (USRSSBPWD) scheme > ----------------------------------------------------------------------------------------- > > Key: DERBY-528 > URL: http://issues.apache.org/jira/browse/DERBY-528 > Project: Derby > Issue Type: New Feature > Components: Security > Affects Versions: 10.1.1.0 > Reporter: Francois Orsini > Assigned To: Francois Orsini > Fix For: 10.2.0.0 > > Attachments: 528_diff_v1.txt, 528_diff_v2.txt, 528_diff_v3.txt, > 528_SecMec_Testing_Table.txt, 528_stat_v1.txt, 528_stat_v2.txt, > 528_stat_v3.txt > > > This JIRA will add support for (DRDA) Strong User ID and Password Substitute > Authentication (USRSSBPWD) scheme in the network client/server driver layers. > Current Derby DRDA network client driver supports encrypted userid/password > (EUSRIDPWD) via the use of DH key-agreement protocol - however current Open > Group DRDA specifications imposes small prime and base generator values (256 > bits) that prevents other JCE's to be used as java cryptography providers - > typical minimum security requirements is usually of 1024 bits (512-bit > absolute minimum) when using DH key-agreement protocol to generate a session > key. > Strong User ID and Password Substitute Authentication (USRSSBPWD) is part of > DRDA specifications as another alternative to provide ciphered passwords > across the wire. > Support of USRSSBPWD authentication scheme will enable additional JCE's to > be used when encrypted passwords are required across the wire. > USRSSBPWD authentication scheme will be specified by a Derby network client > user via the securityMechanism property on the connection UR - A new property > value such as ENCRYPTED_PASSWORD_SECURITY will be defined in order to support > this new (DRDA) authentication scheme. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
