Skip to content
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.

Sign up for GitHub

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

Removing connection properties: fipsProvider and fips #433

Closed
wants to merge 4 commits into from
Closed

Removing connection properties: fipsProvider and fips #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9cd4cc0
Removing connection property - fipsProvider
ulvii Aug 3, 2017
24ffa09
Minor formatting changes
ulvii Aug 3, 2017
8f82d46
Removing fipsProvider from the test
ulvii Aug 3, 2017
ec72a94
Removing connetion properties fipsProvider and fips
ulvii Aug 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 8 additions & 87 deletions src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1577,9 +1577,7 @@ void enableSSL(String host,
String tmfDefaultAlgorithm = null; // Default algorithm (typically X.509) used by the TrustManagerFactory
SSLHandhsakeState handshakeState = SSLHandhsakeState.SSL_HANDHSAKE_NOT_STARTED;

boolean isFips = false;
String trustStoreType = null;
String fipsProvider = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing fipsProvider will not directly impact. But think of following cases:

You can configure BouncyCastle FIPS provider in two ways :

  1. By configuration files (java.security)
  2. Run time by passing FIPS provider .

I added fipsProvider for 2nd case.

Also on Oracle site mentioned following:

Configuring SunJSSE for FIPS Mode

SunJSSE is configured in FIPS mode by associating it with an appropriate FIPS 140 certified cryptographic provider that supplies the implementations for all cryptographic algorithms required by SunJSSE. This can be done in one of the following ways:

  1. edit the file ${java.home}/lib/security/java.security and modify the line that lists com.sun.net.ssl.internal.ssl.Provider to list the provider name of the FIPS 140 certified cryptographic provider. For example if the name of the cryptographic provider is SunPKCS11-NSS, change the line from

    security.provider.4=com.sun.net.ssl.internal.ssl.Provider`

    to

    security.provider.4=com.sun.net.ssl.internal.ssl.Provider SunPKCS11-NSS

    The class for the provider of the given name must also be listed as a security provider in the java.security file.

  2. at runtime, call the constructor of the SunJSSE provider that takes a java.security.Provider object as a parameter. For example, if the variable cryptoProvider is a reference to the cryptographic provider, call new com.sun.net.ssl.internal.ssl.Provider(cryptoProvider).

  3. at runtime, call the constructor of the SunJSSE provider that takes a String object as a parameter. For example if the cryptographic provider is called SunPKCS11-NSS call new com.sun.net.ssl.internal.ssl.Provider("SunPKCS11-NSS"). A provider with the specified name must be one of the configured security providers.

Within a given Java process, SunJSSE can be used either in FIPS mode or in default mode, but not both at the same time. Once SunJSSE has been initialized, it is not possible to change the mode. This means that if one of the runtime configuration options is used (option 2 or 3), the configuration must take place before any SSL/TLS operation.

Ref: Oracle FIPS

Copy link
Contributor Author

@ulvii ulvii Aug 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nsidhaye ,
Thank you so much for detailed comments. We appreciate the time and effort.

As I mentioned above, if fipsProvider is not specified,KeyStore.getInstance() traverses the list of registered security Providers - Security.getProviders() and returns the matching KeyStore object.

Example, in case of option 3, user would make sure BouncyCastleFipsProvider is installed before creating a connection.

Security.addProvider(new org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider());
Security.addProvider(new com.sun.net.ssl.internal.ssl.Provider ("BCFIPS"));
ds.setTrustStoreType("BCFKS");
connection = (SQLServerConnection) ds.getConnection();

When the drivers calls KeyStore.getInstance(trustStoreType) before TLS Handshake, BCFIPS will already be in the list of installed providers.

Please let me know if I am missing something.

Copy link
Contributor Author

@ulvii ulvii Aug 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further investigation, I figured out the root cause of the issue,
Provider IBMJCEFIPS does not contain a keystore. When an application calls ds.setFIPSProvider("IBMJCEFIPS"); , the driver throws an exception, because there are no matching keystore types for IBMJCEFIPS. The users should use non-IBMJCEFIPS keystores to store the keys. See this page. This means, the provider that contains keystore is not necessarily the same as the provider that contains FIPS-complaint algorithms. Below is a sample app that would work for IBM users with the current implementation.

ds.setTrustStoreType("JCEKS");
ds.setFIPS(true);
ds.setFIPSProvider("IBMJCE");  //Not IBMJCEFIPS
connection = (SQLServerConnection) ds.getConnection();

This way we are not introducing any breaking changes, but the connection property fipsProvider has a confusing name.

Copy link
Contributor

@nsidhaye nsidhaye Aug 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ulvi :

In this case as FIPSProvider is non-FIPS it's not fip connection. One can test it by FipsEnvTest#isFips

ds.setTrustStoreType("JCEKS");
ds.setFIPS(true);
ds.setFIPSProvider("IBMJCE"); //Not IBMJCEFIPS
connection = (SQLServerConnection) ds.getConnection();

For one of user I forked out SSLProtocol which having 2 fixes:

  1. Configurable SSL Protocol (Which you already worked on)
  2. Made FIPSProvider optional as IBMFIPS does not like FIPSProvider while calling Keystore.getInstance(...) . (Need to test what happens in JDK 9 & Bouncy Castle for configurable configuration)

Unfortunately, we did not finalize on solution with user, but this worked on user environment with appropriate SSL certificate.

If you use my SSLProtocol fork and use following code for IBM FIPS.

//ds.setTrustStoreType("JCEKS"); (As per my understanding IBM works well with JKS too in FIPS)
ds.setFIPS(true);
//ds.setFIPSProvider("IBMJCE");  For IBM FIPS one should not set FIPSProvider.
connection = (SQLServerConnection) ds.getConnection();

Possibly @v-xiangs might have some information. Feel free to call me.

Copy link
Contributor Author

@ulvii ulvii Aug 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nsidhaye ,
Thank you for the comments. I am definitely open to a discussion over a call. Please email me your contact details at v-ulibra@microsoft.com.

In this case as FIPSProvider is non-FIPS it's not fip connection.

fipsProvider connection property is only used to load the keys. Users will still have to create the keys using a FIPS-approved provider. Non FIPS provider keystores can be used to store those keys and this will not affect FIPS compliant mode. Please, take a look at this article., especially the last section.

One can test it by FipsEnvTest#isFips

Of course this test will fail if you pass in non-FIPS provider. That test just checks whether a provider is FIPS compliant or not, it does not check if FIPS mode is enabled. As I mentioned above, non FIPS provider is used to only store the keys.

If you use my SSLProtocol fork and use following code for IBM FIPS.

Let me explain what happens when you don't set fipsProvider property. In this case, since you did not specify the provider, the driver will traverse the registered providers and find the matching keystore. So if you set keystoreType to JKS, the matching provider that contains this Keystore is a non-FIPS provider. You can put a breakpoint at this line and check the provider name.

Made FIPSProvider optional as IBMFIPS does not like FIPSProvider while calling Keystore.getInstance(...)

Keystore.getInstance(storeType, "IBMJCEFIPS") does not work, because IBMJCEFIPS does not contain any keystores. That's why we need to use non-FIPS provider to store the keys. If the given provider contains the specified storeType, Keystore.getInstance(storeType, provider) is perfectly valid. See this page.

Thanks again for valuable comments and talk to you soon :)


// If anything in here fails, terminate the connection and throw an exception
try {
Expand All @@ -1597,13 +1595,6 @@ void enableSSL(String host,
trustStoreType = SQLServerDriverStringProperty.TRUST_STORE_TYPE.getDefaultValue();
}

fipsProvider = con.activeConnectionProperties.getProperty(SQLServerDriverStringProperty.FIPS_PROVIDER.toString());
isFips = Boolean.valueOf(con.activeConnectionProperties.getProperty(SQLServerDriverBooleanProperty.FIPS.toString()));

if (isFips) {
validateFips(fipsProvider, trustStoreType, trustStoreFileName);
}

assert TDS.ENCRYPT_OFF == con.getRequestedEncryptionLevel() || // Login only SSL
TDS.ENCRYPT_ON == con.getRequestedEncryptionLevel(); // Full SSL

Expand Down Expand Up @@ -1647,12 +1638,8 @@ void enableSSL(String host,
if (logger.isLoggable(Level.FINEST))
logger.finest(toString() + " Finding key store interface");

if (isFips) {
ks = KeyStore.getInstance(trustStoreType, fipsProvider);
}
else {
ks = KeyStore.getInstance(trustStoreType);
}

ks = KeyStore.getInstance(trustStoreType);
ksProvider = ks.getProvider();

// Next, load up the trust store file from the specified location.
Expand Down Expand Up @@ -1710,14 +1697,12 @@ void enableSSL(String host,
tmf.init(ks);
tm = tmf.getTrustManagers();

// if the host name in cert provided use it or use the host name Only if it is not FIPS
if (!isFips) {
if (null != hostNameInCertificate) {
tm = new TrustManager[] {new HostNameOverrideX509TrustManager(this, (X509TrustManager) tm[0], hostNameInCertificate)};
}
else {
tm = new TrustManager[] {new HostNameOverrideX509TrustManager(this, (X509TrustManager) tm[0], host)};
}
// if the host name in cert provided use it or use the host name
Copy link
Contributor

@nsidhaye nsidhaye Aug 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of FIPS our driver should delegate responsibility of validating certificate to JVM.

In this case we are using our own TrustManager i.e. HostNameOverrideX509TrustManager which will throw following exception :

FIPS mode: only SunJSSE TrustManagers may be used

Ref: OpenJDK: SSLContextImpl

If I am correctly remember in order to remove error FIPS mode: only SunJSSE TrustManagers may be used we reviewed our FIPS implementation and added fips attribute.

If you want to get rid of fips attribute then you have to come up with function which will detect if JVM is in FIPS mode or not. You can find logic in our test cases as well as on internet. And then in case of configured FIPS one should not used custom created TrustManagers.

I will suggest instead of merging back in master / dev branch come up with FIPS or FIPSRnD branch and test with different FIPS implementation. If users are comfortable with changes; then merge back in main stream.

Remember to test with at-least following FIPS implementations:

  1. BouncyCastle
  2. IBM
  3. OpenJDK 9 (Not Tested yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out. I am able to reproduce the exception, will investigate and get back to you.

if (null != hostNameInCertificate) {
tm = new TrustManager[] {new HostNameOverrideX509TrustManager(this, (X509TrustManager) tm[0], hostNameInCertificate)};
}
else {
tm = new TrustManager[] {new HostNameOverrideX509TrustManager(this, (X509TrustManager) tm[0], host)};
}
} // end if (!con.trustServerCertificate())

Expand Down Expand Up @@ -1821,70 +1806,6 @@ void enableSSL(String host,
}
}

/**
* Validate FIPS if fips set as true
*
* Valid FIPS settings:
* <LI>Encrypt should be true
* <LI>trustServerCertificate should be false
* <LI>if certificate is not installed FIPSProvider & TrustStoreType should be present.
*
* @param fipsProvider
* FIPS Provider
* @param trustStoreType
* @param trustStoreFileName
* @throws SQLServerException
* @since 6.1.4
*/
private void validateFips(final String fipsProvider,
final String trustStoreType,
final String trustStoreFileName) throws SQLServerException {
boolean isValid = false;
boolean isEncryptOn;
boolean isValidTrustStoreType;
boolean isValidTrustStore;
boolean isTrustServerCertificate;
boolean isValidFipsProvider;

String strError = SQLServerException.getErrString("R_invalidFipsConfig");

isEncryptOn = (TDS.ENCRYPT_ON == con.getRequestedEncryptionLevel());

// Here different FIPS provider supports different KeyStore type along with different JVM Implementation.
isValidFipsProvider = !StringUtils.isEmpty(fipsProvider);
isValidTrustStoreType = !StringUtils.isEmpty(trustStoreType);
isValidTrustStore = !StringUtils.isEmpty(trustStoreFileName);
isTrustServerCertificate = con.trustServerCertificate();

if (isEncryptOn && !isTrustServerCertificate) {
if (logger.isLoggable(Level.FINER))
logger.finer(toString() + " Found parameters are encrypt is true & trustServerCertificate false");

isValid = true;

if (isValidTrustStore) {
// In case of valid trust store we need to check fipsProvider and TrustStoreType.
if (!isValidFipsProvider || !isValidTrustStoreType) {
isValid = false;
strError = SQLServerException.getErrString("R_invalidFipsProviderConfig");

if (logger.isLoggable(Level.FINER))
logger.finer(toString() + " FIPS provider & TrustStoreType should pass with TrustStore.");
}
if (logger.isLoggable(Level.FINER))
logger.finer(toString() + " Found FIPS parameters seems to be valid.");
}
}
else {
strError = SQLServerException.getErrString("R_invalidFipsEncryptConfig");
}

if (!isValid) {
throw new SQLServerException(strError, null, 0, null);
}

}

private final static String SEPARATOR = System.getProperty("file.separator");
private final static String JAVA_HOME = System.getProperty("java.home");
private final static String JAVA_SECURITY = JAVA_HOME + SEPARATOR + "lib" + SEPARATOR + "security";
Expand Down
17 changes: 0 additions & 17 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDataSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,23 +576,6 @@ public boolean getXopenStates() {
return getBooleanProperty(connectionProps, SQLServerDriverBooleanProperty.XOPEN_STATES.toString(),
SQLServerDriverBooleanProperty.XOPEN_STATES.getDefaultValue());
}

public void setFIPS(boolean fips) {
setBooleanProperty(connectionProps, SQLServerDriverBooleanProperty.FIPS.toString(), fips);
}

public boolean getFIPS() {
return getBooleanProperty(connectionProps, SQLServerDriverBooleanProperty.FIPS.toString(),
SQLServerDriverBooleanProperty.FIPS.getDefaultValue());
}

public void setFIPSProvider(String fipsProvider) {
setStringProperty(connectionProps, SQLServerDriverStringProperty.FIPS_PROVIDER.toString(), fipsProvider);
}

public String getFIPSProvider() {
return getStringProperty(connectionProps, SQLServerDriverStringProperty.FIPS_PROVIDER.toString(), null);
}

// The URL property is exposed for backwards compatibility reasons. Also, several
// Java Application servers expect a setURL function on the DataSource and set it
Expand Down
4 changes: 0 additions & 4 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,6 @@ enum SQLServerDriverStringProperty
KEY_STORE_AUTHENTICATION ("keyStoreAuthentication", ""),
KEY_STORE_SECRET ("keyStoreSecret", ""),
KEY_STORE_LOCATION ("keyStoreLocation", ""),
FIPS_PROVIDER ("fipsProvider", ""),
;

private final String name;
Expand Down Expand Up @@ -309,7 +308,6 @@ enum SQLServerDriverBooleanProperty
TRANSPARENT_NETWORK_IP_RESOLUTION ("TransparentNetworkIPResolution", true),
TRUST_SERVER_CERTIFICATE ("trustServerCertificate", false),
XOPEN_STATES ("xopenStates", false),
FIPS ("fips", false),
ENABLE_PREPARE_ON_FIRST_PREPARED_STATEMENT("enablePrepareOnFirstPreparedStatementCall", SQLServerConnection.DEFAULT_ENABLE_PREPARE_ON_FIRST_PREPARED_STATEMENT_CALL);

private final String name;
Expand Down Expand Up @@ -377,9 +375,7 @@ public final class SQLServerDriver implements java.sql.Driver {
new SQLServerDriverPropertyInfo(SQLServerDriverBooleanProperty.XOPEN_STATES.toString(), Boolean.toString(SQLServerDriverBooleanProperty.XOPEN_STATES.getDefaultValue()), false, TRUE_FALSE),
new SQLServerDriverPropertyInfo(SQLServerDriverStringProperty.AUTHENTICATION_SCHEME.toString(), SQLServerDriverStringProperty.AUTHENTICATION_SCHEME.getDefaultValue(), false, new String[] {AuthenticationScheme.javaKerberos.toString(),AuthenticationScheme.nativeAuthentication.toString()}),
new SQLServerDriverPropertyInfo(SQLServerDriverStringProperty.AUTHENTICATION.toString(), SQLServerDriverStringProperty.AUTHENTICATION.getDefaultValue(), false, new String[] {SqlAuthentication.NotSpecified.toString(),SqlAuthentication.SqlPassword.toString(),SqlAuthentication.ActiveDirectoryPassword.toString(),SqlAuthentication.ActiveDirectoryIntegrated.toString()}),
new SQLServerDriverPropertyInfo(SQLServerDriverStringProperty.FIPS_PROVIDER.toString(), SQLServerDriverStringProperty.FIPS_PROVIDER.getDefaultValue(), false, null),
new SQLServerDriverPropertyInfo(SQLServerDriverIntProperty.SOCKET_TIMEOUT.toString(), Integer.toString(SQLServerDriverIntProperty.SOCKET_TIMEOUT.getDefaultValue()), false, null),
new SQLServerDriverPropertyInfo(SQLServerDriverBooleanProperty.FIPS.toString(), Boolean.toString(SQLServerDriverBooleanProperty.FIPS.getDefaultValue()), false, TRUE_FALSE),
new SQLServerDriverPropertyInfo(SQLServerDriverBooleanProperty.ENABLE_PREPARE_ON_FIRST_PREPARED_STATEMENT.toString(), Boolean.toString(SQLServerDriverBooleanProperty.ENABLE_PREPARE_ON_FIRST_PREPARED_STATEMENT.getDefaultValue()), false,TRUE_FALSE),
new SQLServerDriverPropertyInfo(SQLServerDriverIntProperty.SERVER_PREPARED_STATEMENT_DISCARD_THRESHOLD.toString(), Integer.toString(SQLServerDriverIntProperty.SERVER_PREPARED_STATEMENT_DISCARD_THRESHOLD.getDefaultValue()), false, null),
new SQLServerDriverPropertyInfo(SQLServerDriverIntProperty.STATEMENT_POOLING_CACHE_SIZE.toString(), Integer.toString(SQLServerDriverIntProperty.STATEMENT_POOLING_CACHE_SIZE.getDefaultValue()), false, null),
Expand Down
5 changes: 0 additions & 5 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,6 @@ protected Object[][] getContents() {
{"R_keyStoreAuthenticationPropertyDescription", "The name that identifies a key store."},
{"R_keyStoreSecretPropertyDescription", "The authentication secret or information needed to locate the secret."},
{"R_keyStoreLocationPropertyDescription", "The key store location."},
{"R_fipsProviderPropertyDescription", "FIPS Provider."},
{"R_keyStoreAuthenticationNotSet", "\"keyStoreAuthentication\" connection string keyword must be specified, if \"{0}\" is specified."},
{"R_keyStoreSecretOrLocationNotSet", "Both \"keyStoreSecret\" and \"keyStoreLocation\" must be set, if \"keyStoreAuthentication=JavaKeyStorePassword\" has been specified in the connection string."},
{"R_certificateStoreInvalidKeyword", "Cannot set \"keyStoreSecret\", if \"keyStoreAuthentication=CertificateStore\" has been specified in the connection string."},
Expand All @@ -375,10 +374,6 @@ protected Object[][] getContents() {
{"R_TVPnotWorkWithSetObjectResultSet" , "setObject() with ResultSet is not supported for Table-Valued Parameter. Please use setStructured()."},
{"R_invalidQueryTimeout", "The queryTimeout {0} is not valid."},
{"R_invalidSocketTimeout", "The socketTimeout {0} is not valid."},
{"R_fipsPropertyDescription", "Determines if enable FIPS compilant SSL connection between the client and the server."},
{"R_invalidFipsConfig", "Could not enable FIPS."},
{"R_invalidFipsEncryptConfig", "Could not enable FIPS due to either encrypt is not true or using trusted certificate settings."},
{"R_invalidFipsProviderConfig", "Could not enable FIPS due to invalid FIPSProvider or TrustStoreType."},
{"R_serverPreparedStatementDiscardThreshold", "The serverPreparedStatementDiscardThreshold {0} is not valid."},
{"R_statementPoolingCacheSize", "The statementPoolingCacheSize {0} is not valid."},
{"R_kerberosLoginFailedForUsername", "Cannot login with Kerberos principal {0}, check your credentials. {1}"},
Expand Down
Loading