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

COCOS-160 - Enable mTLS when using aTLS #172

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

smithjilks
Copy link
Contributor

What type of PR is this?

This is a feature because it it enables mTLS when using aTLS

What does this do?

  • New Features

    • Introduces mTLS when using aTLS
  • Documentation

    • Added documentation for setting up and managing mutual TLS
  • Tests

    • Added tests to verify TLS functionality and security

Which issue(s) does this PR fix/relate to?

Resolves #160

Have you included tests for your changes?

Yes.

Did you document any new/modified feature?

Yes

Notes

@drasko
Copy link
Contributor

drasko commented Jul 10, 2024

@SammyOina this looks good to me. Is it safe to merge?

@drasko
Copy link
Contributor

drasko commented Jul 10, 2024

@danko-miladinovic please review also

@smithjilks
Copy link
Contributor Author

I need to push a refactor of duplicate code in the switch cases

@smithjilks smithjilks changed the title DRAFT - Enable mTLS when using aTLS COCOS-160 - Enable mTLS when using aTLS Jul 10, 2024
@@ -90,7 +90,7 @@ func (s *Server) Start() error {
creds := grpc.Creds(insecure.NewCredentials())

switch {
case s.Config.AttestedTLS:
case s.Config.AttestedTLS && s.Config.CertFile != "" && s.Config.KeyFile != "":
Copy link
Contributor

Choose a reason for hiding this comment

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

we should make it optional,

  • no tls
  • tls
  • mtls
  • atls
  • matls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SammyOina
default case handles notls
first case handles atls with optional mtls
second case handles atls only
third case handles tls with optional mtls

@drasko
Copy link
Contributor

drasko commented Jul 11, 2024

Fix conflicts, please

@smithjilks smithjilks force-pushed the cocos-160 branch 2 times, most recently from be4a6ec to 610d5b7 Compare July 11, 2024 21:53
Comment on lines 318 to 320
s.Logger.Info(fmt.Sprintf("%s service gRPC server listening at %s with TLS/mTLS cert %s , key %s and %s", s.Name, s.Address, s.Config.CertFile, s.Config.KeyFile, mtlsCA))
default:
s.Logger.Info(fmt.Sprintf("%s service gRPC server listening at %s with TLS cert %s and key %s", s.Name, s.Address, s.Config.CertFile, s.Config.KeyFile))
Copy link
Contributor

Choose a reason for hiding this comment

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

do not print tls cert here

Copy link
Contributor

@SammyOina SammyOina left a comment

Choose a reason for hiding this comment

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

tls connection to agent fails please test

connection error: desc = "transport: authentication handshake failed: read tcp 127.0.0.1:33364->127.0.0.1:6002: read: connection reset by peer

Copy link
Contributor

@SammyOina SammyOina left a comment

Choose a reason for hiding this comment

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

also open a doc on cocos-docs regarding tls configuration for agent with examples

@smithjilks smithjilks force-pushed the cocos-160 branch 2 times, most recently from 69ad474 to 41afa82 Compare August 27, 2024 17:57

tlsConfig.Certificates = append(tlsConfig.Certificates, certificate)
creds = grpc.Creds(credentials.NewTLS(tlsConfig))
s.Logger.Info(fmt.Sprintf("%s service gRPC server listening at %s with Attested TLS", s.Name, s.Address))
Copy link
Contributor

Choose a reason for hiding this comment

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

distinguish this with line 163

Comment on lines +138 to +142
if err != nil {
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of this error check

Copy link
Contributor

@SammyOina SammyOina left a comment

Choose a reason for hiding this comment

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

load certificate fails because of os.stat being unable to read the long filepath in loadx509pair file

{"time":"2024-08-30T00:50:40.571789221+03:00","level":"INFO","msg":"Agent Log/Event, Computation ID: 1, Message: agent_log:{message:\"Transition: receivingAlgorithm -> receivingAlgorithm\\n\"  computation_id:\"1\"  level:\"DEBUG\"  timestamp:{seconds:1724968240  nanos:571937130}}"}
recv 
�
�agent service terminated: failed to load auth certificates: stat -----BEGIN CERTIFICATE-----
MIIDpzCCAo+gAwIBAgIRANvTTwqZC0+8f288fdthx5swDQYJKoZIhvcNAQELBQAw
cTEiMCAGA1UEAwwZVWx0cmF2aW9sZXRfU2VsZnNpZ25lZF9jYTEUMBIGA1UECgwL
VWx0cmF2aW9sZXQxETAPBgNVBAsMCHByaXNtX2NhMSIwIAYJKoZIhvcNAQkBFhNp
bmZvQHVsdHJhdmlvbGV0LnJzMB4XDTI0MDgxNjE0NTAyMFoXDTI0MTExNDE0NTAy
MFowOjEUMBIGA1UEChMLVWx0cmF2aW9sZXQxIjAgBgNVBAMMGVVsdHJhdmlvbGV0
X1NlbGZzaWduZWRfY2EwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCz
lcUc6ky+9jrLJe0gq1�ERROR"
                        ��ö���

�EkiHgvQZwufOwCnMbo9EZDK3wH26HDs5s5FCcp0GuqhUnlQ
pHVzlxipHaNZmeWNpk4EfsIi/VDkqWRt9GSHw9KjlkJ4gqoGd9Th5JcDvmDiejAY
mR7ImWr0yOfT1qkgwd9w9TLrxoFHbSec92E1ll/zl2HhZG0YcmBuU6gA3mpnQtCl
uWJ+4t/6YrGN+ZfPvovvFOAB84Q/VhvKbfWuk1CKQXKtJVX2r2dQKjaXHtE7DJlq
bCXqYazdprEkRic1xKqNhOjaoBWOeQGblJTF03/YXcwBnAw8TXV1TvONfSl6PAZw
48ZY4FbV4ZC1uMPI7Mw9AgMBAAGjcTBvMA4GA1UdDwEB/wQEAwIFoDATBgNVHSUE
DDAKBggrBgEFBQcDAjAMBgNVHRMBAf8EAjAAMB8GA1UdIwQYMBaAFF+9AdUbyw5S
+FBpd5nudwksEtT1MBkGA1UdEQQSMBCCDjEwOS45Mi4xOTUuMT
{"time":"2024-08-30T00:50:40.571814489+03:00","level":"WARN","msg":"EOF"}
{"time":"2024-08-30T00:50:40.571819128+03:00","level":"WARN","msg":"proto: cannot parse invalid wire-format data"}
recv UzMA0GCSqGSI1�ERROR"
                        ��ö���
�
�b3
DQEBCwUAA4IBAQAczXd/9Az4+JvPP7EwWzzLp/PhyfqWi5ostnuuOOXsH3hxp9aG
+/TdTWIs/mSw7X0MXHMOPDyA+ueywaChSt6bxb2MLkQcr+EpL/EnK5sDPMjljUwV
2mb9gQqMlYBZ3W/BF0VhEtYQq0XGnSZrIOe/ip0aHDoZKmv/Hy7gNSHpEb6NOYtS
uQys5lKqG5VJq+U7g4tTjrA0ZcZya2tpEApV6lFs4QTePz3yP9Y1OygeMW+GTWGb
ZLK2/2B2BDldsmfUczy/l2WQItVUMPya40c0nFG8tEGmbiafq2KJ0yaeS93KOFHy
WYID90xH0F63rdFgWEN+V8CvjjtUhLsGi64v
-----END CERTIFICATE-----
: file name too long1�ERROR"

try something like this or modify to be able to load files better, remeber to do the same for CAs

func loadX509KeyPair(certfile, keyfile string) (tls.Certificate, error) {
	var cert, key []byte
	var err error

	readFileOrData := func(input string) ([]byte, error) {
		if len(input) < 1000 && !strings.Contains(input, "\n") {
			data, err := os.ReadFile(input)
			if err == nil {
				return data, nil
			}
		}
		return []byte(input), nil
	}

	cert, err = readFileOrData(certfile)
	if err != nil {
		return tls.Certificate{}, fmt.Errorf("failed to read cert: %v", err)
	}

	key, err = readFileOrData(keyfile)
	if err != nil {
		return tls.Certificate{}, fmt.Errorf("failed to read key: %v", err)
	}

	return tls.X509KeyPair(cert, key)
}

Copy link
Contributor

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

What's the status with this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Enable mTLS when using aTLS
4 participants