Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove gRPC TLS cert #423

Merged
merged 1 commit into from
Jan 26, 2021
Merged

Remove gRPC TLS cert #423

merged 1 commit into from
Jan 26, 2021

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Jan 26, 2021

The way we do TLS in Tinkerbell is just a complication.
It does not increase security because everybody that knows how to
download the certificate from the tink-server can connect.

It can't be used to identify the requester and it means that
is not a carrier that we can use for authorization moving forward.

With a couple of other maintainers, we would like to remove TLS
completely for now until we can figure out a better design for it.

how to migrate

You can update Hegel and boots as well to a versioning that has this patch:

  • hegel (TODO)
  • boots (TODO)

Signed-off-by: Gianluca Arbezzano [email protected]

The way we to TLS in Tinkerbell is just a complication.
It does not increase security because everybody that knows how to
download the certificate from the tink-server can connect.

It can't be used to identify the requester and it means that
is not a carrier that we can use for authorisation moving forward.

With a couple of other maintainers we would like to remove TLS
completely for now until we can figure out a better design for it.

Signed-off-by: Gianluca Arbezzano <[email protected]>
@gianarb gianarb requested a review from mmlb January 26, 2021 13:38
@gianarb gianarb added the breaking-change Denotes a PR that introduces potentially breaking changes that require user action. label Jan 26, 2021
@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #423 (b203e3a) into master (b6c0901) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #423   +/-   ##
=======================================
  Coverage   34.34%   34.34%           
=======================================
  Files          46       46           
  Lines        2865     2865           
=======================================
  Hits          984      984           
  Misses       1794     1794           
  Partials       87       87           

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 b6c0901...b203e3a. Read the comment docs.

@gianarb gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Jan 26, 2021
@mergify mergify bot merged commit c3aca06 into master Jan 26, 2021
@mergify mergify bot deleted the feat/remove-grpc-cert branch January 26, 2021 13:42
@fransvanberckel
Copy link
Contributor

What about removing the generate_certificates function in setup.sh from the sandbox repo?

@gianarb
Copy link
Contributor Author

gianarb commented Jan 26, 2021

Hey @fransvanberckel will do as a follow-up. I want to get the vibe of this #422 first :)

mergify bot added a commit that referenced this pull request Jan 26, 2021
Reverts #423

The `ready-to-merge` label got assigned to early! Sorry @mmlb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Denotes a PR that introduces potentially breaking changes that require user action. ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants