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.

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

WIP :- SSL certificate validation #207

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ashah-splunk
Copy link
Contributor

  • method to set SSL certificate
  • README updated on how to set SSL certificate for verification
  • Test case setup updated with commented code on how to set SSL certificate

- method to add SSL certificate
- README updated on how to set SSL certificate
- Test case updated with commented code using SSL certificate
###SSL Certificate Verification
SSL Certificate validation is turned ON by default in Splunk Java SDK. Set SSL Certificate as shown below.
```java
HttpService.setSSLCert(<byte[] sslCert>);

Choose a reason for hiding this comment

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

This still needs the data input app to provide the cert. How do the data input app figure out which cert to provide here when communicating with the Splunk Enterprise/Cloud ?

@ashah-splunk ashah-splunk requested a review from l00py September 13, 2023 10:31
@@ -109,9 +109,21 @@ To build the documentation for the SDK, it is being automatically generated with
cd splunk
mvn javadoc:javadoc

###SSL Certificate Verification
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space after ### for headers

public static void preClassLoadActions() {
// Bypass the certification validation here.
public static void preClassLoadActions() throws IOException {
// To Bypass the certification validation.
HttpService.setValidateCertificates(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have separate tests:

  1. For simulating the failure of an SSL cert validation
  2. and/or when setting to true, the ability to connect to the HTTPS resource with a valid SSL certificate?

Copy link

@barrerajuanjose barrerajuanjose left a comment

Choose a reason for hiding this comment

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

Hi! My name is JJ from Argentina, I just see the PR and I wanted to leave some comments. It's my first CR in this project, please feel free to ignore my comments if they don't apply. Thanks!

// For now this check is set as null.
// TODO: Implementation logic for validating client certificate.
} else {
if(sslCert == null){

Choose a reason for hiding this comment

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

I would add spaces between if and ( also between ) and {.

if(sslCert == null){
// On initialising before ssCert is set, TM and KM set to null
context.init(null, null, null);
}else{

Choose a reason for hiding this comment

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

I would add spaces between } and else and {

tmf.init(ks);
context.init(null, tmf.getTrustManagers(), null);
}
} else{

Choose a reason for hiding this comment

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

I would add a space between else and {

}else{
InputStream is = new ByteArrayInputStream(sslCert);
CertificateFactory cf = CertificateFactory.getInstance("X.509");
X509Certificate cert = (X509Certificate)cf.generateCertificate(is);

Choose a reason for hiding this comment

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

Do we need this cast to X509Certificate? It looks like the method ks.setCertificateEntry("cert", cert); supports a Certificate in the signature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants