Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Generate a self signed cert on ds creation/update #6024

Merged
rawlinp merged 10 commits into from
Sep 21, 2021
Merged

Generate a self signed cert on ds creation/update #6024

rawlinp merged 10 commits into from
Sep 21, 2021

Conversation

@mattjackson220
Copy link
Contributor

@mattjackson220 mattjackson220 commented Jul 14, 2021

What does this PR (Pull Request) do?

This PR updates TO so that when a new HTTPS delivery service is created or an HTTP delivery service is updated to HTTPS, a self signed certificate is created as a placeholder.

Which Traffic Control components are affected by this PR?

  • Traffic Ops

What is the best way to verify this PR?

Create a new HTTPS DS and verify that a self signed certificate is generated and stored
Create an HTTP DS and verify that no cert is generated
Update that HTTP DS to use HTTPS and verify that a self signed cert is generated and stored
Update an existing HTTPS DS that already has a cert and verify that the previous cert remains unchanged
Verify that all other DS creation / update functionality is unchanged

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

This test does not include docs or tests since it is a minor change that will not impact these

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

@mattjackson220 mattjackson220 added bug something isn't working as intended Traffic Ops related to Traffic Ops improvement The functionality exists but it could be improved in some way. labels Jul 14, 2021
rawlinp
rawlinp suggested changes Jul 16, 2021
View reviewed changes
traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go Outdated
Copy link
Contributor

@rawlinp rawlinp Jul 16, 2021

Choose a reason for hiding this comment

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

Does using inf.Tx.Tx not work?

Copy link
Contributor Author

@mattjackson220 mattjackson220 Jul 22, 2021

Choose a reason for hiding this comment

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

i think it will after the switch to using createV40 and updateV40 so ill get rid of this

Copy link
Contributor

@rawlinp rawlinp Sep 20, 2021

Choose a reason for hiding this comment

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

So I think this went back and forth, but are we not able to use inf.Tx.Tx here? It seems like maybe there was a missing nil pointer check that ended up fixing the tests?

rawlinp
rawlinp reviewed Jul 23, 2021
View reviewed changes
Copy link
Contributor

@rawlinp rawlinp left a comment

Choose a reason for hiding this comment

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

Code looks good, and manual testing confirms the certs are created as expected. Should we add TO API tests to check for the cert creation? It would probably need to be wrapped in if includeSystemTests.

Also, I can't tell if the TO API tests and TP tests are failing due to this PR. I've rerun them a few times, but they seem to be consistent.

@mattjackson220 mattjackson220 force-pushed the SelfSignedDefaultCert branch from 63823b6 to 2615ec7 Compare July 26, 2021 15:36
@smalenfant
Copy link
Contributor

smalenfant commented Aug 1, 2021

Would there be a way to configure these so the CSR is valid to use after being auto-generated? That would mean new configuration.

			BusinessUnit:    util.StrPtr("Placeholder"),
			City:            util.StrPtr("Placeholder"),
			Organization:    util.StrPtr("Placeholder"),
			Country:         util.StrPtr("Placeholder"),
			State:           util.StrPtr("Placeholder"),

@rawlinp
Copy link
Contributor

rawlinp commented Aug 2, 2021

I like that idea. Just make the new config optional and use placeholder by default 👍

@mattjackson220 mattjackson220 force-pushed the SelfSignedDefaultCert branch from 720f4e4 to ee0e574 Compare August 2, 2021 22:02
@mitchell852 mitchell852 modified the milestones: 6.0.0, 6.1.0 Aug 13, 2021
@jhg03a
Copy link
Contributor

jhg03a commented Aug 24, 2021

Closes #2429

@mattjackson220 mattjackson220 force-pushed the SelfSignedDefaultCert branch 2 times, most recently from 870983a to 6c98eaf Compare September 10, 2021 20:07
@mitchell852 mitchell852 modified the milestones: 6.1.0, 6.0.0 Sep 15, 2021
rawlinp
rawlinp suggested changes Sep 20, 2021
View reviewed changes
traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go Outdated
Copy link
Contributor

@rawlinp rawlinp Sep 20, 2021

Choose a reason for hiding this comment

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

So I think this went back and forth, but are we not able to use inf.Tx.Tx here? It seems like maybe there was a missing nil pointer check that ended up fixing the tests?

rawlinp
rawlinp approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@rawlinp rawlinp left a comment

Choose a reason for hiding this comment

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

Code looks good, and TO generated a cert on DS create (with https enabled) and update (from http to https) as expected.

@rawlinp rawlinp merged commit 7d0472d into apache:master Sep 21, 2021
zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Sep 21, 2021
@mattjackson220 @zrhoffman
Generate a self signed cert on ds creation/update (apache#6024)
489f716 
* Generate a self signed cert on ds creation/update

* Added nil check for example urls, fixed test that was failing

* updated per comments

* fixed tests

* more test fixing

* added default cert verification test

* added default cert config to cdn.conf

* updated gofmt

* added nil check

* added default cert config validation and updated db tx use

(cherry picked from commit 7d0472d)
@zrhoffman zrhoffman mentioned this pull request Sep 21, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@rawlinp rawlinp rawlinp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug something isn't working as intended improvement The functionality exists but it could be improved in some way. Traffic Ops related to Traffic Ops

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Snapshotting a CDN that has an HTTPS delivery service w/ no cert causes TR crconfig reload failure

5 participants

@mattjackson220 @smalenfant @rawlinp @jhg03a @mitchell852