Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
common-security: prevent NPE on password protected cert
Motivation:

When attempting to use a password-protected certificate with GridFTP, a NullPointerException is logged without any further information. This is a bug and does not help discover the underlying problem.

Modification:

The CanlContextFactory attempts to get the credential by creating a new instance of PEMCredential, which expects a key password as a char array. Because it is assumed that the key is not password protected, a value of null is passed, leading to a NullPointerException later on. This is changed, for every code occurrence, to instead pass an empty char array, which will result in a more helpful error message.

Result:
A better error message is logged when attempting to use a password-protected credential:
java.io.IOException: Error decrypting private key: the password is incorrect or the PEM data is corrupted.

Target: master
Target: 7.2
Target: 7.1
Target: 7.0
Target: 6.2
Ticket: #10265
Requires-notes: yes
Requires-book: no
Patch: https://rb.dcache.org/r/13337/
Acked-by: Paul Millar
  • Loading branch information
lemora committed Dec 15, 2021
1 parent 824d5a1 commit a937edc
Show file tree
Hide file tree
Showing 8 changed files with 54 additions and 9 deletions.
Expand Up @@ -269,9 +269,14 @@ public void loadingNotification(String location, String type, Severity level,

public Callable<SSLContext> buildWithCaching() throws IOException {
final CanlContextFactory factory = build();
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
Callable<SSLContext> newContext =
() -> factory.getContext(
new PEMCredential(keyPath.toString(), certificatePath.toString(), null));
new PEMCredential(keyPath.toString(), certificatePath.toString(),
new char[]{}));
return memoizeWithExpiration(memoizeFromFiles(newContext, keyPath, certificatePath),
credentialUpdateInterval, credentialUpdateIntervalUnit);
}
Expand Down
Expand Up @@ -186,8 +186,12 @@ private SslContextFactory createDelegate() throws Exception {
// use instance of SslContextFactory.Server as it allows non 'https' protocol schemas.
// See: https://github.com/eclipse/jetty.project/issues/3454
SslContextFactory factory = new SslContextFactory.Server() {
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
private final PEMCredential serverCredential =
new PEMCredential(keyPath.toString(), certificatePath.toString(), null);
new PEMCredential(keyPath.toString(), certificatePath.toString(), new char[]{});

@Override
protected void doStart() throws Exception {
Expand Down
Expand Up @@ -375,9 +375,13 @@ public void start()
caDir).build();
validator = VOMSValidators.newValidator(vomsTrustStore, certChainValidator);

/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
X509Credential credential = new PEMCredential(_properties.getProperty(SERVICE_KEY),
_properties.getProperty(SERVICE_CERT),
null);
new char[]{});
_targetServiceName = convertFromRfc2253(
credential.getCertificate().getSubjectX500Principal().getName(), true);
_targetServiceIssuer = convertFromRfc2253(
Expand Down
12 changes: 10 additions & 2 deletions modules/srm-client/src/main/java/gov/fnal/srm/util/Copier.java
Expand Up @@ -445,10 +445,18 @@ public void copy(CopyJob job) throws Exception {
) {
X509Credential credential;
if (configuration.isUseproxy()) {
credential = new PEMCredential(configuration.getX509_user_proxy(), (char[]) null);
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
credential = new PEMCredential(configuration.getX509_user_proxy(), new char[]{});
} else {
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
credential = new PEMCredential(configuration.getX509_user_key(),
configuration.getX509_user_cert(), null);
configuration.getX509_user_cert(), new char[]{});
}
javaGridFtpCopy(from, to, credential, logger);
} else {
Expand Down
Expand Up @@ -163,11 +163,19 @@ private Optional<X509Credential> getCredential() throws IOException, KeyStoreExc
cred = configuration.getX509_user_proxy() == null
? Optional.<X509Credential>empty()
: Optional.of(
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
new PEMCredential(configuration.getX509_user_proxy(), new char[]{}));
} else {
cred = configuration.getX509_user_key() == null
|| configuration.getX509_user_cert() == null
? Optional.<X509Credential>empty()
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
: Optional.of(new PEMCredential(configuration.getX509_user_key(),
configuration.getX509_user_cert(), new char[]{}));
}
Expand Down
Expand Up @@ -133,7 +133,11 @@ public static void main(String[] arguments) throws Throwable {

public DelegationShell(String proxyPath) throws Exception {
_proxyPath = proxyPath;
_proxy = new PEMCredential(proxyPath, (char[]) null);
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
_proxy = new PEMCredential(proxyPath, new char[]{});

HttpClientSender sender = new HttpClientSender();
Builder contextBuilder = CanlContextFactory.custom();
Expand Down
Expand Up @@ -338,12 +338,20 @@ public SrmShell(URI uri, Args args) throws Exception {
if (configuration.isUseproxy()) {
credential = configuration.getX509_user_proxy() == null
? null
: new PEMCredential(configuration.getX509_user_proxy(), (char[]) null);
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
: new PEMCredential(configuration.getX509_user_proxy(), new char[]{});
} else {
credential = configuration.getX509_user_key() == null
|| configuration.getX509_user_cert() == null
? null
: new PEMCredential(configuration.getX509_user_proxy(), (char[]) null);
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
: new PEMCredential(configuration.getX509_user_proxy(), new char[]{});
}

fs = new AxisSrmFileSystem(decorateWithMonitoringProxy(new Class<?>[]{ISRM.class},
Expand Down
Expand Up @@ -274,7 +274,11 @@ private void write(X509Credential credential, String credentialFileName) {
private X509Credential read(String fileName) {
if (fileName != null) {
try {
return new PEMCredential(fileName, (char[]) null);
/*
* PEMCredential does not consistently support keyPasswd being null
* https://github.com/eu-emi/canl-java/issues/114
*/
return new PEMCredential(fileName, new char[]{});
} catch (IOException | KeyStoreException | CertificateException e) {
LOGGER.error("error reading the credentials from database: {}", e.toString());
}
Expand Down

0 comments on commit a937edc

Please sign in to comment.