[Fix]: Advanced security settings for public repos. #1431

kuhlman-labs · 2022-12-13T15:25:49Z

Resolves #1419

Behavior

Before the change?

Creating a public repo resource would error out if advanced security settings were set with the following message:

"Advanced security is always available for public repos"

After the change?

This change adds logic so that if a repos visibility is set to public it will not include the advanced security field in the payload but still allows setting the other fields.

Other information

Additional info

Pull request checklist

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

Yes (Please add the Type: Breaking change label)
No

If Yes, what's the impact:

N/A

Pull request type

Please add the corresponding label for change this PR introduces:

Bugfix: Type: Bug
Feature/model/API additions: Type: Feature
Updates to docs or samples: Type: Documentation
Dependencies/code cleanup: Type: Maintenance

sethvargo · 2022-12-13T15:41:09Z

github/resource_github_repository.go

-	advancedSecurity := securityAndAnalysis["advanced_security"].([]interface{})[0].(map[string]interface{})
-	update.AdvancedSecurity = &github.AdvancedSecurity{
-		Status: github.String(advancedSecurity["status"].(string)),
+	if d.Get("visibility").(string) != "public" {


I'm not sure this is the correct approach. We should only apply the advanced_security settings if they were explicitly set in the HCL. Right now, setting the value in HCL has no effect (with no warning or anything).

I recommend using GetOk instead of Get on "security_and_analysis" and only adding the value if set.

I considered this approach but I decided to do it this way for a couple of reasons:

The only time where the advanced_security field does not need to be set us for public repositories because it is enabled by default.

In order to use the GetOk approach I would need to set that field as optional which may cause confusion for users in other scenarios.

I think there is some value in seeing that the advanced_security field is set in the HCL even for public repos so practitioners can see at a glance that the repo is set to use advanced security.

The only time where the advanced_security field does not need to be set us for public repositories because it is enabled by default.

That is true as of today. It could change in the future and then suddenly things break in an unexpected way.

In order to use the GetOk approach I would need to set that field as optional which may cause confusion for users in other scenarios.

I'm not sure why you'd need to set it as optional to use GetOk? I also think that's correct because it is optional. It's not required for public or private repositories:

I think there is some value in seeing that the advanced_security field is set in the HCL even for public repos so practitioners can see at a glance that the repo is set to use advanced security.

This is all rooted in the assumption that "public == advanced security analysis", and I think that's a fundamentally flawed assumption.

I think we're talking about two different things here.

I'm not sure why you'd need to set it as optional to use GetOk? I also think that's correct because it is optional. It's not required for public or private repositories:

The block for security_and_analysis is currently optional, if it is not set in the HCL then we don't send the call to configure those settings. However, if you do set the security_and_analysis block in your HCL all of the subfields are required advanced_security, secret_scanning, and secret_scanning_push_protection. The issue when setting the security_and_analysis block for public repos is it would error out because by default advanced_security is enabled and sending that field in the payload would give the error message "Advanced security is always available for public repos". However you can still configure the secret_scanning, and secret_scanning_push_protection features for a public repo. To do a GetOk check I would need to change the advanced_security field to optional which I think would cause some confusion in other scenarios as that field needs to be set to enabled to be able to enable the secret_scanning, and secret_scanning_push_protection fields.

I understand what you're saying, but I don't think this fixes the bug. See

terraform-provider-github/github/resource_github_repository.go

Lines 635 to 644 in 48619d4

} else { // disable security and analysis

_, _, err := client.Repositories.Edit(ctx, owner, repoName, &github.Repository{

SecurityAndAnalysis: &github.SecurityAndAnalysis{

AdvancedSecurity: &github.AdvancedSecurity{

Status: github.String("disabled")},

SecretScanning: &github.SecretScanning{

Status: github.String("disabled")},

SecretScanningPushProtection: &github.SecretScanningPushProtection{

Status: github.String("disabled")}},

})

, as an example. If the github_repository resource was already under management and the user deletes the entire security_and_analysis section, then Terraform will attempt to update the repo to disable Advanced Security (L638). But that's not valid for public repos.

My point is that you're headed down a path where you must explicitly check for repo visibility for any calls that modify security_and_analysis. Not only is this very error-prone, but I think it's the wrong design here.

I've pushed a change that will add some logic to check the repo visibility when removing the security_and_analysis block and send the appropriate payload. Thanks for catching that!

Why do you see the check for visibility as error prone? What approach would you recommend?

Why do you see the check for visibility as error prone?

This comment thread is literally an example of why it's error prone 😄. I think a better pattern is to centralize the logic for compiling the SecurityAndAnalysis block into a single function that takes the current state, previous state, and repo visibility into consideration when building the structure. Then use that function everywhere and throw a ton of unit tests at it to make sure it behaves as expected under all possible conditions.

…urity and analysis block

…lic repos

This currently causes the following error: 422 Advanced security is always available for public repos [] This can be uncommented once this PR [1] is merged and released: [1] integrations/terraform-provider-github#1431 Signed-off-by: Lance Albertson <lance@osuosl.org>

This currently causes the following error: 422 Advanced security is always available for public repos [] This can be uncommented once this PR [1] is merged and released: [1] integrations/terraform-provider-github#1431 Signed-off-by: Lance Albertson <lance@osuosl.org> Signed-off-by: Lance Albertson <lance@osuosl.org>

usmonster · 2023-01-19T09:44:55Z

I guess this is superseded by #1489 and can be closed?

kfcampbell · 2023-01-19T22:58:54Z

@usmonster I believe that is correct, though I'll leave it to @kuhlman-labs to close the PR.

kuhlman-labs mentioned this pull request Dec 13, 2022

[MAINT]: Public Repositories do not play well with Advanced Security settings #1419

Closed

1 task

sethvargo suggested changes Dec 13, 2022

View reviewed changes

kuhlman-labs added 4 commits December 14, 2022 09:24

adding logic to check if repo is public and then send appropriate sec…

71b16bb

…urity and analysis block

adding logic to handle removal of security_and_analysis block for pub…

d04362a

…lic repos

adding check for security_and_analysis opts when removing block

b509736

Merge branch 'main' into maint/1419_repo_adv_security

4ca1a1e

swissgiz added a commit to swissgiz/terraform-provider-github that referenced this pull request Dec 19, 2022

trying integrations#1431 as quick fix

7b0b73a

This was referenced Dec 19, 2022

quickfix #1445

Closed

Fix advanced security settings for public repos swissgiz/terraform-provider-github#1

Merged

nickfloyd added Type: Bug Something isn't working as documented Priority: Normal labels Jan 3, 2023

ramereth mentioned this pull request Jan 11, 2023

Disable security_and_analysis block due to failures sous-chefs/terraform-github-repository#36

Merged

3 tasks

Merge branch 'main' into maint/1419_repo_adv_security

748309b

kuhlman-labs closed this Jan 26, 2023

kuhlman-labs deleted the maint/1419_repo_adv_security branch January 26, 2023 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix]: Advanced security settings for public repos. #1431

[Fix]: Advanced security settings for public repos. #1431

kuhlman-labs commented Dec 13, 2022

sethvargo Dec 13, 2022

kuhlman-labs Dec 13, 2022

sethvargo Dec 13, 2022

kuhlman-labs Dec 13, 2022

sethvargo Dec 14, 2022

kuhlman-labs Dec 14, 2022 •

edited

sethvargo Dec 14, 2022

usmonster commented Jan 19, 2023

kfcampbell commented Jan 19, 2023

	} else { // disable security and analysis
	_, _, err := client.Repositories.Edit(ctx, owner, repoName, &github.Repository{
	SecurityAndAnalysis: &github.SecurityAndAnalysis{
	AdvancedSecurity: &github.AdvancedSecurity{
	Status: github.String("disabled")},
	SecretScanning: &github.SecretScanning{
	Status: github.String("disabled")},
	SecretScanningPushProtection: &github.SecretScanningPushProtection{
	Status: github.String("disabled")}},
	})

[Fix]: Advanced security settings for public repos. #1431

[Fix]: Advanced security settings for public repos. #1431

Conversation

kuhlman-labs commented Dec 13, 2022

Behavior

Before the change?

After the change?

Other information

Additional info

Pull request checklist

Does this introduce a breaking change?

Pull request type

sethvargo Dec 13, 2022

Choose a reason for hiding this comment

kuhlman-labs Dec 13, 2022

Choose a reason for hiding this comment

sethvargo Dec 13, 2022

Choose a reason for hiding this comment

kuhlman-labs Dec 13, 2022

Choose a reason for hiding this comment

sethvargo Dec 14, 2022

Choose a reason for hiding this comment

kuhlman-labs Dec 14, 2022 • edited

Choose a reason for hiding this comment

sethvargo Dec 14, 2022

Choose a reason for hiding this comment

usmonster commented Jan 19, 2023

kfcampbell commented Jan 19, 2023

kuhlman-labs Dec 14, 2022 •

edited