Skip to content

Commit

Permalink
minio: Set secure=true to enable TLS by default (#3168)
Browse files Browse the repository at this point in the history
* minio: Set secure=true to enable TLS by default

Not using TLS is a security concern, especially if using cloud storage
like S3. This should be set to secure to avoid people unknowingly not
using TLS.

To make the bundled minio still work, I've submitted
kubeflow/manifests#950 to set secure=false in
this case explicitly.

* minio: secure=false in GCP & standalone manifests
  • Loading branch information
discordianfish authored Feb 27, 2020
1 parent 74e0593 commit 5cb158d
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 1 deletion.
2 changes: 1 addition & 1 deletion backend/src/apiserver/client_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ func initMinioClient(initConnectionTimeout time.Duration) storage.ObjectStoreInt
minioServiceRegion := common.GetStringConfigWithDefault(
"ObjectStoreConfig.Region", os.Getenv(minioServiceRegion))
minioServiceSecure := common.GetBoolConfigWithDefault(
"ObjectStoreConfig.Secure", common.GetBoolFromStringWithDefault(os.Getenv(minioServiceSecure), false))
"ObjectStoreConfig.Secure", common.GetBoolFromStringWithDefault(os.Getenv(minioServiceSecure), true))
accessKey := common.GetStringConfigWithDefault("ObjectStoreConfig.AccessKey", "")
secretKey := common.GetStringConfigWithDefault("ObjectStoreConfig.SecretAccessKey", "")
bucketName := common.GetStringConfigWithDefault("ObjectStoreConfig.BucketName", os.Getenv(pipelineBucketName))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,8 @@ spec:
{{ if .Values.managedstorage.enabled }}
- name: OBJECTSTORECONFIG_BUCKETNAME
value: '{{ tpl .Values.managedstorage.gcsBucketName . }}'
- name: OBJECTSTORECONFIG_SECURE
value: "false"
- name: DBCONFIG_DBNAME
{{ if .Values.managedstorage.databaseNamePrefix }}
value: '{{ .Values.managedstorage.databaseNamePrefix }}_pipeline'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ spec:
valueFrom:
fieldRef:
fieldPath: metadata.namespace
- name: OBJECTSTORECONFIG_SECURE
value: "false"
image: gcr.io/ml-pipeline/api-server:0.1.27
imagePullPolicy: IfNotPresent
name: ml-pipeline-api-server
Expand Down

3 comments on commit 5cb158d

@rmgogogo
Copy link
Contributor

@rmgogogo rmgogogo commented on 5cb158d Feb 28, 2020

Choose a reason for hiding this comment

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

this CL enabled it as default yes but the input config doesn't work (not pass into the golang)

I will revert this PR. Feel free to send out another one again and cc me for a review. I will add my self to BE reviewer list.

@Bobgy
Copy link
Contributor

@Bobgy Bobgy commented on 5cb158d Feb 28, 2020

Choose a reason for hiding this comment

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

5cb158d#diff-c1706b818573ecac9f26168961383394R669
this line was wrong, it should be outside {{ if .Values.managedstorage.enabled }}, managedstorage.enabled will use gcs for minio artifacts, so it should be true, if not managed storage, we need it to be false.

@rmgogogo
Copy link
Contributor

Choose a reason for hiding this comment

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

#3191 reverted it.
I will patch it in another PR after 0.2.5 release (working on it).

Please sign in to comment.