COCOS-160 - Enable mTLS when using aTLS #172
Open
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
|
@SammyOina this looks good to me. Is it safe to merge? |
|
@danko-miladinovic please review also |
|
I need to push a refactor of duplicate code in the switch cases |
smithjilks
changed the title
SammyOina
requested changes
Jul 10, 2024
internal/server/grpc/grpc.go
Outdated
| @@ -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 != "": | |||
There was a problem hiding this comment.
we should make it optional,
- no tls
- tls
- mtls
- atls
- matls
There was a problem hiding this comment.
@SammyOina
default case handles notls
first case handles atls with optional mtls
second case handles atls only
third case handles tls with optional mtls
|
Fix conflicts, please |
smithjilks
force-pushed
the
cocos-160
branch
2 times, most recently
from
be4a6ec to
610d5b7
Compare
SammyOina
requested changes
Jul 31, 2024
internal/server/grpc/grpc.go
Outdated
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)) |
There was a problem hiding this comment.
do not print tls cert here
SammyOina
requested changes
Aug 19, 2024
SammyOina
reviewed
Aug 19, 2024
smithjilks
force-pushed
the
cocos-160
branch
2 times, most recently
from
69ad474 to
41afa82
Compare
SammyOina
reviewed
Aug 29, 2024
|
|
||
| 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)) |
There was a problem hiding this comment.
distinguish this with line 163
SammyOina
reviewed
Aug 29, 2024
Comment on lines
+138
to
+142
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
what is the purpose of this error check
SammyOina
requested changes
Aug 29, 2024
There was a problem hiding this comment.
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)
}
smithjilks
force-pushed
the
cocos-160
branch
from
41afa82 to
6b8ec2f
Compare
dborovcanin
reviewed
Sep 20, 2024
Signed-off-by: Jilks Smith <[email protected]>
Signed-off-by: Jilks Smith <[email protected]>
smithjilks
force-pushed
the
cocos-160
branch
from
6b8ec2f to
e8cd004
Compare
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.
What type of PR is this?
This is a feature because it it enables mTLS when using aTLS
What does this do?
New Features
Documentation
Tests
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