Stop serving and fetching the tls cert used for gRPC #584
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #584 +/- ##
==========================================
- Coverage 46.22% 45.86% -0.36%
==========================================
Files 56 56
Lines 3310 3268 -42
==========================================
- Hits 1530 1499 -31
+ Misses 1690 1686 -4
+ Partials 90 83 -7
Continue to review full report at Codecov.
|
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
mmlb
force-pushed
the
drop-cert
branch
2 times, most recently
from
fa05ac2 to
b1e4f5b
Compare
mmlb
force-pushed
the
drop-cert
branch
5 times, most recently
from
4af365c to
b885e17
Compare
mmlb
added a commit
to mmlb/tinkerbell-hook
that referenced
this pull request
Feb 14, 2022
See tinkerbell/tink#567 and tinkerbell/tink#584. Signed-off-by: Manuel Mendez <[email protected]>
|
@mmlb looks like this will need a rebase and to follow along with @micahhausler's refactoring done in #598 |
mmlb
force-pushed
the
drop-cert
branch
2 times, most recently
from
bfab656 to
23596ec
Compare
micahhausler
suggested changes
Apr 7, 2022
This was only ever a hack that was moderately ok when it was done in Cacher when running in an old EM orchestration stack. IT was ok because we *always* fetched the cert via a "normal" tls connection via a tls terminating proxy. This was not guaranteed in tinkerbell land and actually exists insecurely in the sandbox at the moment (see sandbox fetching the cert via http). Having this here made self-signed certs "easier" but also less safe while making the normal and desired behavior more difficult. The normal being tinkerbell using cert generated by a trusted CA. This way certificate revokation can actually happen, where as with the /cert method all clients would have to redownload the /cert after figuring out the cert is no longer valid (somehow). This should never have made it into tinkerbell from cacher. Signed-off-by: Manuel Mendez <[email protected]>
Now that we don't need the certificate in pem format nor the cert's modification time we can drop a whole lot of complexity and use more functionality from the imported libs. Signed-off-by: Manuel Mendez <[email protected]>
They no longer matter. Signed-off-by: Manuel Mendez <[email protected]>
It's just one member so might as well drop it in favor of just passing value as an arg. Signed-off-by: Manuel Mendez <[email protected]>
It's just two members so might as well drop it in favor of just passing values as a args. Signed-off-by: Manuel Mendez <[email protected]>
Get rid of init() func and the function it calls too. I find it much easier to understand the code flow and how viper/cobra interact by setting evertying up in Execute. Signed-off-by: Manuel Mendez <[email protected]>
Previously rootCmd's cobra.Command had not been able to run and thus the command line arguments weren't yet parsed by the time we were connecting to tinkerbell. Environment vars would work which is how we usually set everything up so went undetected. By waiting until PreRun we give cobra a chance to parse the command line and thus we do the right thing. Signed-off-by: Manuel Mendez <[email protected]>
It really is. Signed-off-by: Manuel Mendez <[email protected]>
We don't generate certificates in tink-server any more. Signed-off-by: Manuel Mendez <[email protected]>
markjacksonfishing
suggested changes
Apr 18, 2022
Signed-off-by: Manuel Mendez <[email protected]>
For easier back-and-forth mental-modelling. Signed-off-by: Manuel Mendez <[email protected]>
Aborting from withing a library package...., should never have been committed! Signed-off-by: Manuel Mendez <[email protected]>
3 tasks
This was referenced Apr 18, 2022
This was referenced Apr 27, 2022
mergify bot
added a commit
to tinkerbell/smee
that referenced
this pull request
May 4, 2022
## Description I was getting ready to PR my branch that is updated to work in /cert-less tink (now that tinkerbell/tink#584 is merged) and it was basically a situation where pulling one thread unraveled a bunch of other things. Dropping /cert begot some refactors (started by dropping the `buildWorkerParams` silliness) which begot some test fixes which begot some other refactors which was also dumb to do for rancher, nixos, and centos. So here we are. ## Why is this needed Makes updating to post tinkerbell/tink#584 easier/nicer. ## How Has This Been Tested? Ran unit tests. ## How are existing users impacted? What migration steps/scripts do we need? Less EM specific junk is always good. Less duplicate/unclear tests too. This PR does introduce a change wrt sandbox, where MIRROR_HOST is dropped in favor of MIRROR_BASE_URL, I'll follow up with a PR there once this merges. ## Checklist: I have: - [ ] updated the documentation and/or roadmap (if required) - [ ] added unit or e2e tests - [ ] provided instructions on how to upgrade
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
These were used to get around having lots of parameters and the long lines that caused, but now are just one or two values.
Why is this needed
The whole serving of the gRPC certificate via http as
/certshould never have made it to tinkerbell.It's too easy to use incorrectly and fall into a sense of security that may not be there.
Its also a pain to actually use in production when following modern best practices of short lived TLS certificates.
Clients can't use gRPC's/Go's builtin certificate handling of when certs are rotated.
This is not a lot of good, yet a bunch of bad only to make a corner case easier at the expense of normal route.
/certmakes self-sigend certs "easy"ish (see cert rotation issue still) but it's just not worth it.If someone can handle the implications of self-signed certs in production (#567 can be used for dev envs) then they can figure out how to embed their CA into hook or roll their own tink-worker environment.
How Has This Been Tested?
Compiles and tests pass.
How are existing users impacted? What migration steps/scripts do we need?
This is a breaking change for out-of-tree clients, I think its worth it compared to the ops / security benefits.
Fixes #324