Tinkerbell GRPC Insecure Mode #567

nshalman · 2021-12-06T20:59:51Z

Description

This PR, along with some minor cleanup, enables tink-server to run without a TLS certificate and enables tink-cli to connect to it without needing TLS to be set up.

At the time of opening the PR, this change is intended to only extend the capabilities of tink-server and tink-cli rather than fundamentally altering them, though I am open to the latter before this lands if that's the right answer.

Why is this needed

This will simplify development and testing of the tinkerbell stack, and could theoretically be used in the sandbox.
Alternately, the sandbox could include TLS termination similar to how many applications use Kubernetes ingress for TLS termination.

How Has This Been Tested?

Template creation and retrieval

cat focal.yaml | docker exec -i tink_tink-cli_1 tink template create
docker exec -i tink_tink-cli_1 tink template get

against make run-stack
with the sandbox

How are existing users impacted? What migration steps/scripts do we need?

This is intended to not impact

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

codecov · 2021-12-06T21:05:30Z

Codecov Report

Merging #567 (8282ae7) into main (fc76c03) will increase coverage by 0.02%.
The diff coverage is 71.42%.

❗ Current head 8282ae7 differs from pull request most recent head ae0df5f. Consider uploading reports for the commit ae0df5f to get more accurate results

@@            Coverage Diff             @@
##             main     #567      +/-   ##
==========================================
+ Coverage   43.39%   43.41%   +0.02%     
==========================================
  Files          51       51              
  Lines        3104     3107       +3     
==========================================
+ Hits         1347     1349       +2     
- Misses       1671     1673       +2     
+ Partials       86       85       -1

Impacted Files	Coverage Δ
grpc-server/grpc_server.go	`36.48% <71.42%> (+1.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc76c03...ae0df5f. Read the comment docs.

tobert · 2021-12-06T21:31:58Z

client/main.go

+	conn, err := grpc.Dial(opt.GRPCAuthority,
+		method,
+		grpc.WithUnaryInterceptor(otelgrpc.UnaryClientInterceptor()),
+		grpc.WithStreamInterceptor(otelgrpc.StreamClientInterceptor()),


excellent :)

Might want to add a client option to print the trace id in this case e.g. in the logs, so it's easy to take to a search box.

Do you have an example elsewhere of how to do that I can crib from?

tobert · 2021-12-06T21:33:25Z

client/main.go

+	method := grpc.WithInsecure()
+	insecure := os.Getenv("TINKERBELL_INSECURE")
+
+	if insecure == "" {


no big deal but I usually make flags like insecure here go through strconv.ParseBool, technically safer than allowing even " " to get interpreted as true

this also implies TINKERBELL_INSECURE=0 means to run in insecure mode...

+1 to being more explicit.

client/main.go

stephen-fox · 2021-12-06T21:28:57Z

cmd/tink-server/main.go

@@ -93,6 +95,11 @@ func (c *DaemonConfig) PopulateFromLegacyEnvVar() {
 	if basicAuthPass := os.Getenv("TINK_AUTH_PASSWORD"); basicAuthPass != "" {
 		c.HTTPBasicAuthPassword = basicAuthPass
 	}
+	if insecure, isSet := os.LookupEnv("INSECURE"); isSet {


I think we should use TINKERBELL_INSECURE for consistency, and to avoid possibly conflicting with another application.

Good catch. Thanks!

Maybe that should be TINK_INSECURE instead of TINKERBELL_?

stephen-fox · 2021-12-06T21:32:19Z

cmd/tink-server/main.go

@@ -93,6 +95,11 @@ func (c *DaemonConfig) PopulateFromLegacyEnvVar() {
 	if basicAuthPass := os.Getenv("TINK_AUTH_PASSWORD"); basicAuthPass != "" {
 		c.HTTPBasicAuthPassword = basicAuthPass
 	}
+	if insecure, isSet := os.LookupEnv("INSECURE"); isSet {
+		if b, err := strconv.ParseBool(insecure); err != nil {


Should this be err == nil?

Or, should this result in a panic?

You could also ignore the error I suppose, as that will result in what I consider to be a decent default: that a TLS listener gets created.

I think I cribbed this from ONLY_MIGRATION above. a bug in both locations? Did any of this ever work??

It's probably time to collapse the two different codepaths for env vars and command line flags into one piece of correct code... :sigh:

stephen-fox · 2021-12-06T21:33:50Z

cmd/tink-server/main.go

+			if config.Insecure {
+				rpcServer.SetupGRPC(ctx, logger, &rpcServer.ConfigGRPCServer{
+					Facility:      config.Facility,
+					TLSCert:       "insecure",


Can we leave this field as unspecified / default value (empty string)?

"insecure" is being used as a sentinel value to skip tls stuff in rpcServer.SetupGRPC. I'll be cleaning that up next and this will all go away then. In the meantime I don't think its worth re-working this + setupGRPC if its going to be deleted soon.

docker-compose-insecure.yaml

stephen-fox · 2021-12-06T21:37:32Z

client/main.go

 }

-func NewClientConn(opt *ConnOptions) (*grpc.ClientConn, error) {
-	resp, err := http.Get(opt.CertURL)
+func fetchCert(url string) (credentials.TransportCredentials, error) {


I think the function name could be a bit more descriptive. Maybe grpcCredentialFromCertEndpoint, or trustedCertsFromCertEndpoint.

Either that, or a comment would be helpful.

whynotboth.gif

food for thought on naming: https://talks.golang.org/2014/names.slide#3

stephen-fox · 2021-12-06T21:39:55Z

client/main.go

@@ -90,8 +71,23 @@ func NewClientConn(opt *ConnOptions) (*grpc.ClientConn, error) {
 		return nil, errors.Wrap(err, "parse cert")


I know we didn't create this issue, but I just realized that err here is nil.

An improvement would be decoding the certificate's PEM and parsing the resulting ASN.1 DER using the x509 library. That, or creating a new error that says something like failed to parse cert - cert pool append certs from pem failed.

I'd agree, but I'm actually looking to drop the /cert completely so this will go away anyway.

stephen-fox · 2021-12-06T21:54:26Z

grpc-server/grpc_server.go

@@ -60,14 +60,19 @@ func SetupGRPC(ctx context.Context, logger log.Logger, config *ConfigGRPCServer,
 		dbReady: true,
 		logger:  logger,
 	}
-	if cert := config.TLSCert; cert != "" {


Could we add a field to ConfigGRPCServer that specified no TLS?

The original if statement might work as-is... But it does leave some ambiguity about what should happen if the field is an empty string.

I'll be reworking this soon and should do a better job of dealing with tls certs. Lets ignore this for now please.

micahhausler

I'm trying to better understand the motivations for this PR. Is it that "generating TLS serving certs is hard"? Or is passing arounds certs trusted by the server is difficult?

My read of this PR is that we want to provide Tink server operators the ability to turn off client TLS authentication on the server.

Can the docs be updated with this new mode and the flags to turn it on?

Insecure can have multiple meanings:

No TLS at all (client or server or both)
Still use mTLS, but trust any TLS certificate

Can the "insecure" parameters be named more specifically? Maybe something like insecureNoTLSAuthentication

micahhausler · 2021-12-07T15:29:25Z

client/main.go

+	method := grpc.WithInsecure()
+	insecure := os.Getenv("TINKERBELL_INSECURE")
+
+	if insecure == "" {


+1 to being more explicit.

micahhausler · 2021-12-07T15:37:58Z

client/main.go

-	creds := credentials.NewClientTLSFromCert(cp, "")
+
+	method := grpc.WithInsecure()
+	insecure := os.Getenv("TINKERBELL_INSECURE")


Since we're using pflag above, can we use Viper to get the env var?

should be addressed

micahhausler · 2021-12-07T19:26:08Z

docker-compose-insecure.yaml

+      PGUSER: tinkerbell
+      TINKERBELL_GRPC_AUTHORITY: :42113
+      TINKERBELL_HTTP_AUTHORITY: :42114
+      TINKERBELL_INSECURE: "true"


Do we need a full new docker-compoose? Can this not default to false and accept an override for true?

Agree, I'll give this a shot

I've gotten rid of the extra/unnecessary docker-compose file

nshalman · 2021-12-07T21:15:46Z

My read of this PR is that we want to provide Tink server operators the ability to turn off client TLS authentication on the server.

We want to turn off TLS as the transport, largely to simplify development, but possibly also for production uses.
On the server side we don't want to use TLS.
On the client side we want the GRPC client connection to be created using the "WithInsecure" DialOption which "disables transport security" (see https://pkg.go.dev/google.golang.org/grpc#WithInsecure)

Can the docs be updated with this new mode and the flags to turn it on?

If you mean docs within this repo, that's certainly a reasonable thing to request before this is approved, though we should probably resolve the naming issues first.
I'd rather not write docs elsewhere before this is approved, though it seems reasonable to me that they be written before it lands.

Insecure can have multiple meanings:

No TLS at all (client or server or both)

Still use mTLS, but trust any TLS certificate

Can the "insecure" parameters be named more specifically? Maybe something like insecureNoTLSAuthentication

I chose "insecure" because that's what GRPC calls the DialOption. I guess "insecureNoTLS" could work, but at that rate we could call it "noTLS" too.

micahhausler · 2021-12-07T21:44:02Z

We want to turn off TLS as the transport, largely to simplify development, but possibly also for production uses.
On the server side we don't want to use TLS.

Introducing a new mode where users can hurt themselves is a bit scary to me, even if it is prefixed with "insecure." Can we simplify development and deployment with certificate generating scripts/more documentation?

Edit: If the primary motivation is development, can we use build tags to make development easier, but release artifacts are don't use the insecure mode?

I chose "insecure" because that's what GRPC calls the DialOption. I guess "insecureNoTLS" could work, but at that rate we could call it "noTLS" too.

If we do go down this route, I'm a fan of insecureNoTLS/TINKERBELL_INSECURE_NO_TLS/--insecure-no-tls. If/when other security features are added (ex: authorization), you'll want to differentiate them and still make it explicit that the configuration option is insecure.

mmlb · 2022-01-18T19:32:44Z

I'm going to take over this PR so will be dusting this off and responding to feedback.

mmlb · 2022-01-18T19:50:54Z

I have 2 primary reasons for wanting to see this PR through. First is to enable a simpler dev mode where we don't paper over the insecure setup necessary for sandbox (https://github.com/tinkerbell/sandbox/blob/9a04338168efd85be173100eb165bd2d3c6d09c2/deploy/compose/docker-compose.yml#L78). The second reason is to do away with the whole /cert thing. /cert came around from when cacher was first developed and had to deal with internal stuff. It should have never made its way into tinkerbell. The only reason it worked in cacher and sort of works in tinkerbell is by piggy-backing on tls to fetch /cert. We can't do that in sandbox with the self-signed certs and so the one safety anchor goes away.

IMO tinkerbell should support tls via an ingress or tls-terminating proxy (and thus runs in insecure mode itself, so the traffic is in the clear once "on-prem") or if you want to have tls for tinkerbell all the way through we should pass in valid/trusted-ca certs and do away with self-signed certs all together. The former is a nice middle ground of safe-enough for many use cases (remember we do want to be homelab-ish friendly too) and the latter could still be used to support self-signed tls anyway, its just harder. I'm ok with stating "if you already want to deal with the hard work of a self-signed pki infrastructure, then you should be ready to roll your own live environment to handle your 'untrsuted CA' (or embed the CA cert into hook.. somehow".

mmlb · 2022-01-18T19:58:20Z

We want to turn off TLS as the transport, largely to simplify development, but possibly also for production uses.
On the server side we don't want to use TLS.

Introducing a new mode where users can hurt themselves is a bit scary to me, even if it is prefixed with "insecure." Can we simplify development and deployment with certificate generating scripts/more documentation?

This should be doable I think in sandbox code, by repackaging hook's initrd to include the generated CA's cert into the trust store, but I don't want to do it myself. I'd rather spend our time on better tinkerbell features than supporting self-signed certs easiliy.

Edit: If the primary motivation is development, can we use build tags to make development easier, but release artifacts are don't use the insecure mode?

Its not just development, that was my primary want but after thinking some more on this I think "insecure" mode is actually secure enough for many users. Standing up a tls terminating grpc proxy that does clear-text to its backends is very common, especially in homelab/messing-around scenarios.

I chose "insecure" because that's what GRPC calls the DialOption. I guess "insecureNoTLS" could work, but at that rate we could call it "noTLS" too.

If we do go down this route, I'm a fan of insecureNoTLS/TINKERBELL_INSECURE_NO_TLS/--insecure-no-tls. If/when other security features are added (ex: authorization), you'll want to differentiate them and still make it explicit that the configuration option is insecure.

I like this, but would rather have it be --no-tls. We can be secure w/o tinkerbell server running tls itself.

mmlb · 2022-01-27T21:36:23Z

Alright this should be the last of the changes. I've changed the args and code from negative to positive --no-tls/TINKERBELL_NO_TLS (default false) -> --tls/TINKERBELL_TLS (default true). This avoided needing to have double negatives in the code (if !noTLS for example). Still depends on #583 and as such has its commits, but should be cleared up soon.

## Description Drop all code dealing with json rpc. ## Why is this needed Its likely unused code which has some weird code (:eye: runtime.String) that make other PRs more tedious (#567, also I want to drop the self-signed `/cert` stuff too) to implement. ## How Has This Been Tested? `go build` and CI. ## How are existing users impacted? What migration steps/scripts do we need? No impact I suspect (and hope). I know of 3 other tink clients and they all use grpc: - https://github.com/swills/py-tink-cli - https://github.com/micahhausler/tink/tree/feature/k8s-mode/cmd/virtual-worker - an Equinix Metal internal service

This is useful for local development and should never be used in production environments. Signed-off-by: Nahum Shalman <[email protected]> Signed-off-by: Manuel Mendez <[email protected]>

Helps for visually scanning. Signed-off-by: Manuel Mendez <[email protected]>

We already import packethost/pkg might as well make the most of it. Signed-off-by: Manuel Mendez <[email protected]>

NewClientConn already exists to make the grpc connection, no reason not to use it. Signed-off-by: Manuel Mendez <[email protected]>

Do not setup tls in the grpc dial opts and do not setup /cert path. Signed-off-by: Manuel Mendez <[email protected]>

Sort services. Sort env bars. Sort depends_on services. Reorder defs so they are ordered in->out, e.g.: image/build first followed by entrypoint/cmd finishing with things needed to talk to it idk just wanted to have a similar order to compare services. Signed-off-by: Manuel Mendez <[email protected]>

/cert does not exist when in non-tls mode. Also, why use /cert when /healthz exists and is made for exactly for this. Signed-off-by: Manuel Mendez <[email protected]>

We already import packethost/pkg might as well make the most of it. Signed-off-by: Manuel Mendez <[email protected]>

Signed-off-by: Nahum Shalman <[email protected]> Signed-off-by: Manuel Mendez <[email protected]>

Signed-off-by: Manuel Mendez <[email protected]>

mmlb · 2022-02-10T15:09:16Z

Alrighty then, now that #583 is merged I've rebased this so the diff is minimal again. Looking from some review from @micahhausler @nshalman @stephen-fox 🙏

See tinkerbell/tink#567 and tinkerbell/tink#584. Signed-off-by: Manuel Mendez <[email protected]>

## Description - Stops serving the gRPC cert via http and stops fetching it. - Cleans up a ton of code that was confusing to read in the clients due to dealing with the gRPC certificate. - Removes a few Config structs that are passed into funcs 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 `/cert` should 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. `/cert` makes 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

nshalman force-pushed the insecure-mode branch 3 times, most recently from 28cdc1e to 0a19809 Compare December 6, 2021 21:16

nshalman marked this pull request as ready for review December 6, 2021 21:16

nshalman requested a review from stephen-fox December 6, 2021 21:17

ScottGarman self-requested a review December 6, 2021 21:30

tobert reviewed Dec 6, 2021

View reviewed changes

nshalman commented Dec 6, 2021

View reviewed changes

client/main.go Outdated Show resolved Hide resolved

stephen-fox suggested changes Dec 6, 2021

View reviewed changes

micahhausler suggested changes Dec 7, 2021

View reviewed changes

micahhausler reviewed Dec 7, 2021

View reviewed changes

nshalman force-pushed the insecure-mode branch from 43ee9d2 to f9d074f Compare December 7, 2021 21:14

nshalman added the do-not-merge Signal to Mergify to block merging of the PR. label Dec 7, 2021

nshalman marked this pull request as draft December 8, 2021 19:12

nshalman force-pushed the insecure-mode branch 4 times, most recently from a1926dd to 0c2f2f4 Compare December 10, 2021 16:44

mmlb force-pushed the insecure-mode branch 2 times, most recently from ee9088f to 8246789 Compare January 18, 2022 21:23

mmlb marked this pull request as ready for review January 19, 2022 16:03

mmlb force-pushed the insecure-mode branch 2 times, most recently from f8d8636 to 9039869 Compare January 24, 2022 18:23

mmlb force-pushed the insecure-mode branch from 1ca6ca3 to e1cab25 Compare January 27, 2022 21:31

mmlb mentioned this pull request Jan 27, 2022

Stop serving and fetching the tls cert used for gRPC #584

Merged

jacobweinstock force-pushed the insecure-mode branch from 1ca6ca3 to 9c7ac1c Compare February 8, 2022 16:44

nshalman and others added 10 commits February 10, 2022 10:06

client: support connecting to GRPC servers without TLS

8f71358

This is useful for local development and should never be used in production environments. Signed-off-by: Nahum Shalman <[email protected]> Signed-off-by: Manuel Mendez <[email protected]>

docs: Sort envvars table

12e37dc

Helps for visually scanning. Signed-off-by: Manuel Mendez <[email protected]>

client: Use packethost/pkg/env helper functions instead of stdlib

4af9470

We already import packethost/pkg might as well make the most of it. Signed-off-by: Manuel Mendez <[email protected]>

client: refactor GetConnection to use NewClientConn

c91e7b8

NewClientConn already exists to make the grpc connection, no reason not to use it. Signed-off-by: Manuel Mendez <[email protected]>

http: Do the right thing if no cert is passed to SetupHTTP

aba55c1

Do not setup tls in the grpc dial opts and do not setup /cert path. Signed-off-by: Manuel Mendez <[email protected]>

Use /healthz for docker-compose healthcheck

be6b4ac

/cert does not exist when in non-tls mode. Also, why use /cert when /healthz exists and is made for exactly for this. Signed-off-by: Manuel Mendez <[email protected]>

tink-server: Use packethost/pkg/env helper functions instead of stdlib

ddde0da

We already import packethost/pkg might as well make the most of it. Signed-off-by: Manuel Mendez <[email protected]>

tink-server: support GRPC insecure mode

a21393b

Signed-off-by: Nahum Shalman <[email protected]> Signed-off-by: Manuel Mendez <[email protected]>

docs: Add TINKERBELL_TLS ENVVARS.md

ae0df5f

Signed-off-by: Manuel Mendez <[email protected]>

mmlb force-pushed the insecure-mode branch from 9c7ac1c to ae0df5f Compare February 10, 2022 15:06

mmlb added do-not-merge Signal to Mergify to block merging of the PR. ready-to-merge Signal to Mergify to merge the PR. and removed do-not-merge Signal to Mergify to block merging of the PR. labels Feb 10, 2022

mmlb self-assigned this Feb 10, 2022

mmlb removed the do-not-merge Signal to Mergify to block merging of the PR. label Feb 10, 2022

markjacksonfishing approved these changes Feb 10, 2022

View reviewed changes

mmlb removed request for ScottGarman, micahhausler and stephen-fox February 11, 2022 14:40

mergify bot merged commit 1337c42 into tinkerbell:main Feb 11, 2022

mmlb added a commit to mmlb/tinkerbell-hook that referenced this pull request Feb 14, 2022

Update to handle non-tls and no cert-url tink server

a060aeb

See tinkerbell/tink#567 and tinkerbell/tink#584. Signed-off-by: Manuel Mendez <[email protected]>

displague added this to the 0.7.0 milestone Aug 15, 2022

		@@ -90,8 +71,23 @@ func NewClientConn(opt ConnOptions) (grpc.ClientConn, error) {
		return nil, errors.Wrap(err, "parse cert")

Tinkerbell GRPC Insecure Mode #567

Tinkerbell GRPC Insecure Mode #567

Conversation

nshalman commented Dec 6, 2021 • edited by mmlb Loading

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

codecov bot commented Dec 6, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahhausler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nshalman commented Dec 7, 2021 • edited Loading

micahhausler commented Dec 7, 2021 • edited Loading

mmlb commented Jan 18, 2022

mmlb commented Jan 18, 2022

mmlb commented Jan 18, 2022

mmlb commented Jan 27, 2022

mmlb commented Feb 10, 2022

nshalman commented Dec 6, 2021 •

edited by mmlb

Loading

codecov bot commented Dec 6, 2021 •

edited

Loading

nshalman commented Dec 7, 2021 •

edited

Loading

micahhausler commented Dec 7, 2021 •

edited

Loading