Add `--tls-certificate-secret-name` parameter to server command. Fixes #5582 #9423

chtcvl · 2022-08-24T16:34:27Z

Signed-off-by: vladimir ivanov [email protected]

Add --tls-certificate-secret-name parameter to server command.
Since server runs on localhost:2746, get an error in the UI "certificate is valid for ... not for localhost". Therefore the merge request set InsecureSkipVerify on TLSconfig for grpc client on the gateway.

Tested to to work with secret provided by cert-manager (added to manifests).

juliev0

Interesting. Thanks for the addition. Just wondering if we should support the case in which somebody doesn't want the InsecureSkipVerify? (Don't want to add more unnecessary work but not sure what others think)

juliev0 · 2022-08-31T23:04:24Z

manifests/base/argo-server/argo-server-certificate.yaml

+  issuerRef:
+    kind: Issuer
+    name: argo-workflows-issuer
+  secretName: argo-server-tls


Did you intend for this to be deployed as part of installing our manifests? I see manifests/base/argo-server/kustomization.yaml calls out the yaml files in this directory. Should we add it to that?

Thanks, added to manifests/base/argo-server/kustomization.yaml.

Yes, intend to install as part manifests, since certificate from Certificate resource is persistent compared to one argo-server generates itself. This way user will only need to set up trust once.

chtcvl · 2022-09-01T02:19:09Z

Interesting. Thanks for the addition. Just wondering if we should support the case in which somebody doesn't want the InsecureSkipVerify? (Don't want to add more unnecessary work but not sure what others think)

This InsecureSkipVerify is for communication within the same instance of argo-server application - between grpc client and server goroutines, so don't expect this requirement.

juliev0 · 2022-09-01T05:02:41Z

Interesting. Thanks for the addition. Just wondering if we should support the case in which somebody doesn't want the InsecureSkipVerify? (Don't want to add more unnecessary work but not sure what others think)

This InsecureSkipVerify is for communication within the same instance of argo-server application - between grpc client and server goroutines, so don't expect this requirement.

Oh, I see…

juliev0 · 2022-09-01T05:32:18Z

Details

Great. As far as I can tell, that will propagate into all of the various manifests. You'll need to run "make codegen -B STATIC_FILES=false" and "git diff" and check in whatever changed (which you can see here).

juliev0

Thanks for updating the manifests. As far as I can tell, the change looks good to me, but unfortunately I'm not too knowledgeable about this area of the code and hoping someone more knowledgeable can review.

juliev0 · 2022-09-13T15:06:53Z

@sarabala1979 @terrytangyuan Are either of you able to review and merge this PR?

terrytangyuan · 2022-09-13T15:24:07Z

I am not familiar with this. Maybe @alexec @sarabala1979?

sarabala1979 · 2022-09-13T16:59:00Z

server/apiserver/argoserver.go

@@ -334,7 +334,10 @@ func (as *argoServer) newHTTPServer(ctx context.Context, port int, artifactServe
 		grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(MaxGRPCMessageSize)),
 	}
 	if as.tlsConfig != nil {
-		dialOpts = append(dialOpts, grpc.WithTransportCredentials(credentials.NewTLS(as.tlsConfig)))
+		tlsConfig := as.tlsConfig
+		tlsConfig.InsecureSkipVerify = true


@chtcvl Do you need to make InsecureSkipVerify=true for all cases? Can you make as command line arg and configure in tsConfig?

Right now gRPC server explicitly runs on localhost (

argo-workflows/server/apiserver/argoserver.go

Line 260 in 769896e

url = "https://localhost" + address

) meaning https://localhost. For grpc-gateway to access gRPC server it has some options:
1 . set InsecureSkipVerify=true (not really a concern as this is communication between two goroutines in the same app) - let grpc-gateway trust gRPC server running on localhost.
2. have certificate Subject Alternate Name include localhost - this is not supported by publicly trusted certificates.
3. serve on https://realdomain... for which certificate is issued instead of localhost.

This being said, adding new option for InsecureSkipVerify=false can lead to confusion as option will only be usable in pt.3 which is not supported by current code.

Thanks for your response. LGTM @alexec Can you quickly take a look? if I am missing anything.

Signed-off-by: vladimir.ivanov <[email protected]>

…rgoproj#5582 (argoproj#9423) fix: Add --tls-certificate-secret-name parameter to server command Signed-off-by: vladimir.ivanov <[email protected]> Signed-off-by: vladimir.ivanov <[email protected]> Signed-off-by: yangxue.chen <[email protected]>

Piroddi · 2022-10-02T08:44:04Z

Hi, This PR enforces users of the manifests to have the cert-manger CRD's installed. Is this the intention?

To use the new manifests (while happy with a self signed cert). Had to add the following to kustomize:

patchesStrategicMerge:
  - |-
    apiVersion: cert-manager.io/v1
    kind: Issuer
    metadata:
      name: argo-workflows-issuer
    $patch: delete
  - |-
    apiVersion: cert-manager.io/v1
    kind: Certificate
    metadata:
      name: argo-server-cert
    $patch: delete

allemp · 2022-10-03T08:52:53Z

Hi, This PR enforces users of the manifests to have the cert-manger CRD's installed. Is this the intention?

To use the new manifests (while happy with a self signed cert). Had to add the following to kustomize:

This was a huge issue for me as well.

…Fixes #5582 (#9423)" This reverts commit ff6aab3.

…rgoproj#5582 (argoproj#9423) fix: Add --tls-certificate-secret-name parameter to server command Signed-off-by: vladimir.ivanov <[email protected]> Signed-off-by: vladimir.ivanov <[email protected]> Signed-off-by: juchao <[email protected]>

chtcvl force-pushed the add-tls-certificate-secret-name-key branch 3 times, most recently from d2e0dd4 to 8bae7c5 Compare August 27, 2022 11:19

juliev0 reviewed Aug 31, 2022

View reviewed changes

chtcvl force-pushed the add-tls-certificate-secret-name-key branch from 8bae7c5 to dbbc822 Compare September 1, 2022 02:22

chtcvl force-pushed the add-tls-certificate-secret-name-key branch from dbbc822 to 2858919 Compare September 1, 2022 09:11

chtcvl requested a review from juliev0 September 2, 2022 01:06

chtcvl force-pushed the add-tls-certificate-secret-name-key branch from fe9c125 to c065a95 Compare September 8, 2022 06:00

juliev0 requested a review from sarabala1979 September 8, 2022 22:47

juliev0 reviewed Sep 8, 2022

View reviewed changes

chtcvl force-pushed the add-tls-certificate-secret-name-key branch 2 times, most recently from 5132b89 to c5b73e1 Compare September 13, 2022 06:18

terrytangyuan closed this Sep 13, 2022

terrytangyuan reopened this Sep 13, 2022

sarabala1979 reviewed Sep 13, 2022

View reviewed changes

chtcvl force-pushed the add-tls-certificate-secret-name-key branch from c5b73e1 to 69f759c Compare September 14, 2022 10:31

fix: Add --tls-certificate-secret-name parameter to server command

c642000

Signed-off-by: vladimir.ivanov <[email protected]>

chtcvl force-pushed the add-tls-certificate-secret-name-key branch from 69f759c to c642000 Compare September 21, 2022 08:25

sarabala1979 approved these changes Sep 27, 2022

View reviewed changes

sarabala1979 merged commit ff6aab3 into argoproj:master Sep 27, 2022

allemp mentioned this pull request Oct 3, 2022

Cert manager suddenly a dependency??? #9737

Closed

3 tasks

terrytangyuan added a commit that referenced this pull request Oct 6, 2022

Revert "Add --tls-certificate-secret-name parameter to server command. …

abd4dad

…Fixes #5582 (#9423)" This reverts commit ff6aab3.

terrytangyuan mentioned this pull request Oct 6, 2022

Revert "Add --tls-certificate-secret-name parameter to server command. Fixes #5582" #9756

Merged

chtcvl mentioned this pull request Oct 12, 2022

fix: Add --tls-certificate-secret-name parameter to server command. Fixes #5582 #9789

Merged

8 tasks

agilgur5 added type/security Security related area/manifests area/server labels Aug 17, 2024

agilgur5 changed the title ~~Add --tls-certificate-secret-name parameter to server command. Fixes #5582~~ Add --tls-certificate-secret-name parameter to server command. Fixes #5582 Aug 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--tls-certificate-secret-name` parameter to server command. Fixes #5582 #9423

Add `--tls-certificate-secret-name` parameter to server command. Fixes #5582 #9423

chtcvl commented Aug 24, 2022 •

edited by agilgur5

Loading

juliev0 left a comment

juliev0 Aug 31, 2022

chtcvl Sep 1, 2022

chtcvl commented Sep 1, 2022

juliev0 commented Sep 1, 2022

juliev0 commented Sep 1, 2022

juliev0 left a comment

juliev0 commented Sep 13, 2022

terrytangyuan commented Sep 13, 2022

sarabala1979 Sep 13, 2022

chtcvl Sep 14, 2022

sarabala1979 Sep 18, 2022

Piroddi commented Oct 2, 2022 •

edited by agilgur5

Loading

allemp commented Oct 3, 2022 •

edited by agilgur5

Loading

Add --tls-certificate-secret-name parameter to server command. Fixes #5582 #9423

Add --tls-certificate-secret-name parameter to server command. Fixes #5582 #9423

Conversation

chtcvl commented Aug 24, 2022 • edited by agilgur5 Loading

juliev0 left a comment

Choose a reason for hiding this comment

juliev0 Aug 31, 2022

Choose a reason for hiding this comment

chtcvl Sep 1, 2022

Choose a reason for hiding this comment

chtcvl commented Sep 1, 2022

juliev0 commented Sep 1, 2022

juliev0 commented Sep 1, 2022

juliev0 left a comment

Choose a reason for hiding this comment

juliev0 commented Sep 13, 2022

terrytangyuan commented Sep 13, 2022

sarabala1979 Sep 13, 2022

Choose a reason for hiding this comment

chtcvl Sep 14, 2022

Choose a reason for hiding this comment

sarabala1979 Sep 18, 2022

Choose a reason for hiding this comment

Piroddi commented Oct 2, 2022 • edited by agilgur5 Loading

allemp commented Oct 3, 2022 • edited by agilgur5 Loading

Add `--tls-certificate-secret-name` parameter to server command. Fixes #5582 #9423

Add `--tls-certificate-secret-name` parameter to server command. Fixes #5582 #9423

chtcvl commented Aug 24, 2022 •

edited by agilgur5

Loading

Piroddi commented Oct 2, 2022 •

edited by agilgur5

Loading

allemp commented Oct 3, 2022 •

edited by agilgur5

Loading