S3 disable ssl verify #131
Conversation
...and hope minio creds can use the AWS creds object
NewWithRegion doesn't seem to like the (url, &minio.Options) call. Try using the older (documented) style with this for setting a custom transport: minio/minio-go#1019
Adapted from aws/aws-sdk-go#2404
Still need to add flag to actual http.NewRequest call in StoreFile()
NewS3FileStore() takes an additional boolean argument for disableSSLVerify. Hardcode `false` to each of those calls in the test suite.
For example, when using a self-signed certificate, disable verifying the cert in the PUT of a new object.
Need crypto/tls to specify custom transport config.
instead of DefaultClient (unsure what the real difference is if http.Client() just starts with a DefaultClient)
Codecov Report
@@ Coverage Diff @@
## s3-disable-ssl-verify #131 +/- ##
=========================================================
+ Coverage 91.14% 91.20% +0.05%
=========================================================
Files 14 14
Lines 1412 1421 +9
=========================================================
+ Hits 1287 1296 +9
Misses 92 92
Partials 33 33
Continue to review full report at Codecov.
|
| TLSClientConfig: &tls.Config{InsecureSkipVerify: fs.disableSSLverify}, | ||
| } | ||
| // Timeout: time.Second * 10, | ||
| httpClient := &http.Client{ |
There was a problem hiding this comment.
Here we're creating a new client for every request, vs. http.DefaultClient which reuses a client. Do the client docs have any advice about this? The Java client recommends you share one client per process, for example.
If we want to share a client, I'd just create the client in the build code and replace the disableSSLverify arg with the client.
| @@ -83,7 +83,7 @@ func (t *TestSuite) TestConstructWithGoodBucketNames() { | |||
| ls := b.String() | |||
| t.Equal(63, len(ls), "incorrect string length") | |||
| for _, bucket := range []string{"foo", ls} { | |||
| fstore, err := NewS3FileStore(cli, min, bucket) | |||
| fstore, err := NewS3FileStore(cli, min, bucket, false) | |||
There was a problem hiding this comment.
I would disable ssl for one of the happy path tests so the code is exercised, if not tested completely, and add a note to the top of the test file to that effect.
| awscli := s3.New(sess, &aws.Config{ | ||
| Credentials: creds, | ||
| Endpoint: &cfg.S3Host, | ||
| Region: &cfg.S3Region, | ||
| DisableSSL: &cfg.S3DisableSSL, | ||
| HTTPClient: customHTTPClient, |
There was a problem hiding this comment.
I'm worried here that we're blowing away any specific http client customizations the aws and minio clients do. Are there any docs about that?
There was a problem hiding this comment.
Not that I could find.
| @@ -35,6 +35,10 @@ const ( | |||
| // KeyS3DisableSSL is the configuration key that determines whether SSL is to be used. | |||
| // any value other than 'true' is treated as false. | |||
| KeyS3DisableSSL = "s3-disable-ssl" | |||
| // KeyS3DisableSSLVerify is the configuration key that determines whether to verify the | |||
There was a problem hiding this comment.
Can you add tests for the new code here to https://github.com/kbase/blobstore/blob/master/config/config_test.go
Add a configuration option (default to false) that disables verification of the S3 SSL certificate (for example, if it is self-signed).