Remove SAML connector and tests by camscale · Pull Request #18285 · gravitational/teleport

camscale · 2022-11-09T00:18:22Z

Remove the SAML connector from the OSS repository. It has been migrated
to the enterprise repository.

Issue: https://github.com/gravitational/teleport.e/issues/525

NOTE: This PR should be reviewed in conjunction with https://github.com/gravitational/teleport.e/pull/597
which adds the SAML connector to the enterprise repo.

ravicious · 2022-11-16T15:50:54Z

 	"github.com/stretchr/testify/require"
 )

-func FuzzParseSAMLInResponseTo(f *testing.F) {


I'm not familiar with this part of the codebase, but are we okay with removing this fuzz test completely? From what I see, it wasn't moved to teleport.e.

The reference to this test can be also found in

teleport/fuzz/oss-fuzz-build.sh

Line 30 in fd4097b

FuzzParseSAMLInResponseTo fuzz_parse_saml_in_response_to

So probably we need to remove it from there.

Ah yes. I over-zealously removed this (the always-failing CIFuzz annoys me no end), and forgot to see what I needed to do to move it to the new repo. I'll get onto that.

oss-fuzz-build.sh fixed in 340df18

I've added the fuzz test to the enterprise PR. As yet there is nothing to run it as there is no CI job there to do fuzz testing. I can't figure out how the fuzz/oss-fuzz-build.sh script is meant to work - I think I'm missing the context in which it runs - there look to be undefined functions and variables, so I suspect OSSFuzz provides this context and corpora. A similar setup will need to be done on the enterprise repo.

@reedloden Is there any feedback you can add?

OSS-Fuzz runs fuzz/oss-fuzz-build.sh from the config in https://github.com/google/oss-fuzz/tree/master/projects/teleport.

OSS-Fuzz only works on open source projects. We can use https://fuzzbuzz.io/ for teleport.e, but it's not configured yet. As long as you ensure Go-native fuzzing is set up (by way of the fuzz test existing), when we do get Fuzzbuzz working, it will Just Work(TM).

smallinsky · 2022-11-16T16:27:16Z

 	"github.com/stretchr/testify/require"
 )

-func FuzzParseSAMLInResponseTo(f *testing.F) {


The reference to this test can be also found in

teleport/fuzz/oss-fuzz-build.sh

Line 30 in fd4097b

FuzzParseSAMLInResponseTo fuzz_parse_saml_in_response_to

So probably we need to remove it from there.

smallinsky · 2022-11-16T16:28:27Z

 	if err := a.authConnectorAction(apidefaults.Namespace, types.KindSAML, types.VerbUpdate); err != nil {
 		return trace.Wrap(err)
 	}
-	if !modules.GetModules().Features().SAML {


Wonder if there is a case when a client can have a Enterprise product without SAML feature ?

Not currently as far as I'm aware. This check is still performed but in a different way. If the enterprise startup does not register the SAML connector, you'll still get this error message from the code in lib/auth/saml.go in the OSS repo. If we do get a scenario where we can have an enterprise product without SAML, we can revise the error message then since I don't know how to word it otherwise right now.

camscale · 2022-11-21T04:30:42Z

@ravicious @smallinsky This PR has a failing test for the operator that I did not notice before as it is not run from make test-go on the command line. I've raised #18634 to keep some auth/SAML methods in auth.Server. Once I get that merged, I'll rebase this and the other PR on top of that. I think some of the tests that moved across will end up staying now as those related to Upsert/Delete of the connector will be fully implemented in this OSS repo.

Remove the SAML connector code as it now lives in the enterprise repository. All that is left behind is a small forwarding stub that returns a NotImplemented error if a SAMLService is not plugged into the auth.Server.

Remove the SAML tests that were not in `saml_test.go`. These tests have already been moved to the enterprise repository along with the SAML connector.

This fuzz test has moved to the enterprise repository, so remove the run from here.

Add `TestSAMLValidation` back to `grpcserver_test.go` as it does not do actual SAML auth, just creating a connector. That is fully supported in OSS - only the auth path required enterprise.

Add a "fmt" import that got lost in a rebase.

camscale force-pushed the camh/remove-saml branch from 7347563 to 64c009d Compare November 9, 2022 00:23

camscale force-pushed the camh/sso-to-e branch from 9f6129e to 3fce221 Compare November 9, 2022 00:32

camscale force-pushed the camh/remove-saml branch 2 times, most recently from b52d83f to 7e9fb1f Compare November 9, 2022 06:31

camscale mentioned this pull request Nov 9, 2022

auth: Split SAML connector code to own type #18255

Merged

camscale force-pushed the camh/sso-to-e branch from 3fce221 to 663901b Compare November 14, 2022 04:06

camscale force-pushed the camh/remove-saml branch from 7e9fb1f to 0a4058b Compare November 14, 2022 04:06

camscale force-pushed the camh/sso-to-e branch 2 times, most recently from 22d628c to e6a0d70 Compare November 16, 2022 02:50

Base automatically changed from camh/sso-to-e to master November 16, 2022 03:24

camscale force-pushed the camh/remove-saml branch 2 times, most recently from 2cc2f7e to 468004b Compare November 16, 2022 06:47

camscale marked this pull request as ready for review November 16, 2022 11:13

github-actions Bot requested review from ravicious and smallinsky November 16, 2022 11:14

ravicious reviewed Nov 16, 2022

View reviewed changes

smallinsky reviewed Nov 16, 2022

View reviewed changes

ravicious approved these changes Nov 18, 2022

View reviewed changes

smallinsky approved these changes Nov 18, 2022

View reviewed changes

camscale mentioned this pull request Nov 21, 2022

auth: Remove Upsert/Delete from SAMLService interface #18634

Merged

camscale force-pushed the camh/remove-saml branch 2 times, most recently from b4173ea to e0051ff Compare November 21, 2022 06:54

camscale changed the base branch from master to camh/narrow-saml-interface November 21, 2022 06:57

camscale force-pushed the camh/narrow-saml-interface branch from 0ea5f94 to 550a864 Compare November 24, 2022 04:29

camscale added 4 commits November 25, 2022 12:46

auth: Remove SAML connector code

2f5f32a

Remove the SAML connector code as it now lives in the enterprise repository. All that is left behind is a small forwarding stub that returns a NotImplemented error if a SAMLService is not plugged into the auth.Server.

auth: Remove remaining SAML tests

cdd1ff2

Remove the SAML tests that were not in `saml_test.go`. These tests have already been moved to the enterprise repository along with the SAML connector.

fuzz: Remove run of FuzzParseSAMLInResponseTo

6d7eef3

This fuzz test has moved to the enterprise repository, so remove the run from here.

auth: Add back TestSAMLValidation

497cf28

Add `TestSAMLValidation` back to `grpcserver_test.go` as it does not do actual SAML auth, just creating a connector. That is fully supported in OSS - only the auth path required enterprise.

camscale force-pushed the camh/remove-saml branch from e0051ff to e1efb93 Compare November 25, 2022 01:47

camscale changed the base branch from camh/narrow-saml-interface to master November 25, 2022 02:06

auth: Add missing "fmt" import

7bbf73d

Add a "fmt" import that got lost in a rebase.

camscale force-pushed the camh/remove-saml branch from 92dc420 to 8a38cb5 Compare November 25, 2022 02:12

Update enterprise submodule

5cea218

camscale force-pushed the camh/remove-saml branch from 8a38cb5 to 5cea218 Compare November 25, 2022 03:57

Merge branch 'master' into camh/remove-saml

fa2c465

camscale merged commit 45642fb into master Nov 28, 2022

camscale mentioned this pull request Jan 14, 2023

[v11] Backport removal of enterprise SSO connectors #20244

Closed

camscale deleted the camh/remove-saml branch April 6, 2023 06:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove SAML connector and tests#18285

Remove SAML connector and tests#18285
camscale merged 7 commits intomasterfrom
camh/remove-saml

camscale commented Nov 9, 2022 •

edited

Loading

Uh oh!

ravicious Nov 16, 2022

Uh oh!

smallinsky Nov 16, 2022

Uh oh!

camscale Nov 16, 2022

Uh oh!

camscale Nov 17, 2022

Uh oh!

camscale Nov 17, 2022

Uh oh!

reedloden Nov 17, 2022

Uh oh!

smallinsky Nov 16, 2022

Uh oh!

smallinsky Nov 16, 2022

Uh oh!

camscale Nov 16, 2022

Uh oh!

camscale commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

camscale commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camscale commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

camscale commented Nov 9, 2022 •

edited

Loading