NOISSUE - Refactor CSR generation #55

nyagamunene · 2024-11-28T16:30:38Z

What type of PR is this?

This is a refactor because it changes the following functionality: it removes CSR repository.

What does this do?

It removes CSR repository.

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

N/A

Have you included tests for your changes?

Yes

Did you document any new/modified features?

No

Notes

Signed-off-by: nyagamunene <[email protected]>

SammyOina

Method should be IssueFromCSR with csr as argument and the response should be the issued certificate

Signed-off-by: nyagamunene <[email protected]>

SammyOina · 2024-12-02T10:28:50Z

api/http/endpoint.go

@@ -314,71 +314,38 @@ func createCSREndpoint(svc certs.Service) endpoint.Endpoint {
 	return func(ctx context.Context, request interface{}) (response interface{}, err error) {
 		req := request.(createCSRReq)
 		if err := req.validate(); err != nil {
-			return createCSRRes{created: false}, err


what is the purpose of create csr endpoint, users can create csrs using openssl

Its an alternative if the user doesn't want to use openssl. The idea is to make certs more generic.

move to cli tool, no need to call an online api to create a CSR

Signed-off-by: nyagamunene <[email protected]>

SammyOina · 2024-12-02T12:52:56Z

api/http/endpoint.go

 		}

-		csr, err := svc.CreateCSR(ctx, req.Metadata, req.Metadata.EntityID, req.privKey)
+		cert, err := svc.IssueFromCSR(ctx, req.entityID, req.ttl, certs.CSR{CSR: []byte(req.CSR), PrivateKey: []byte(req.PrivateKey)})


why is the private key uploaded,

you don't need the private key when signing the certificate, you only need the public key. the point of this endpoint is for users who want to keep their private keys secure

Let me make that change

Signed-off-by: nyagamunene <[email protected]>

dborovcanin · 2024-12-06T13:00:34Z

cli/certs.go

+	case *rsa.PrivateKey:
+		signer = key
+	case *ecdsa.PrivateKey:
+		signer = key
+	case ed25519.PrivateKey:
+		signer = key


Group cases:

case *rsa.PrivateKey, *ecdsa.PrivateKey, *ed25519.PrivateKey:

dborovcanin · 2024-12-06T13:01:17Z

cli/certs.go

+		}
+		return CreateCSR(metadata, parsedKey)
+	default:
+		return certs.CSR{}, errors.NewSDKError(errors.Wrap(ErrCreateEntity, errors.New("unsupported private key type")))


Extract errors.New("unsupported...") to a variable.

dborovcanin · 2024-12-06T13:03:02Z

cli/certs.go

+		privateKey, err = x509.ParsePKCS1PrivateKey(block.Bytes)
+	case certs.ECPrivateKey:
+		privateKey, err = x509.ParseECPrivateKey(block.Bytes)
+	case certs.PrivateKey, "PKCS8 PRIVATE KEY":


Extract string literal to constant. Please double-check to see if there is no such constant somewhere in imported packages.

dborovcanin · 2024-12-06T13:03:09Z

cli/certs.go

+		privateKey, err = x509.ParseECPrivateKey(block.Bytes)
+	case certs.PrivateKey, "PKCS8 PRIVATE KEY":
+		privateKey, err = x509.ParsePKCS8PrivateKey(block.Bytes)
+	case "ED25519 PRIVATE KEY":


dborovcanin · 2024-12-06T13:03:24Z

cli/certs.go

+	case "ED25519 PRIVATE KEY":
+		privateKey, err = x509.ParsePKCS8PrivateKey(block.Bytes)
+	default:
+		err = errors.New("unsupported private key type")


Use error var.

dborovcanin · 2024-12-06T13:03:36Z

cli/certs.go

+		err = errors.New("unsupported private key type")
+	}
+	if err != nil {
+		return nil, errors.New("failed to parse key")


Extract error to var.

Signed-off-by: nyagamunene <[email protected]>

dborovcanin · 2024-12-09T09:06:02Z

service.go

+// The certificate is then stored in the repository using the CreateCert method.
+// If the root CA is not found, it returns an error.
+func (s *service) IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, options SubjectOptions, key ...any) (Certificate, error) {
+	var privKey any


Use var privKey crypto.PrivateKey instead.

dborovcanin · 2024-12-09T09:08:39Z

certs.go

@@ -217,7 +158,7 @@ type Service interface {
 	RetrieveCAToken(ctx context.Context) (string, error)

 	// IssueCert issues a certificate from the database.
-	IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, option SubjectOptions, privKey ...*rsa.PrivateKey) (Certificate, error)
+	IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, option SubjectOptions, privKey ...any) (Certificate, error)


Use privKey crypto.PrivateKey instead.

Signed-off-by: nyagamunene <[email protected]>

dborovcanin · 2024-12-09T09:44:50Z

certs.go

@@ -217,7 +159,7 @@ type Service interface {
 	RetrieveCAToken(ctx context.Context) (string, error)

 	// IssueCert issues a certificate from the database.
-	IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, option SubjectOptions, privKey ...*rsa.PrivateKey) (Certificate, error)
+	IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, option SubjectOptions, privKey ...crypto.PrivateKey) (Certificate, error)


Is there a reason for this to be a variadic function? Can we just use have

IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, option SubjectOptions, privKey crypto.PrivateKey) (Certificate, error)

Yes there is a reason. It because its also been used by the issueCSR method to issue certificates since they user needs to provide their own private key

Signed-off-by: nyagamunene <[email protected]>

dborovcanin · 2024-12-09T10:00:01Z

service.go

+// issueCert generates and issues a certificate for a given backendID.
+// It uses the RSA algorithm to generate a private key, and then creates a certificate
+// using the provided template and the generated private key.
+// The certificate is then stored in the repository using the CreateCert method.
+// If the root CA is not found, it returns an error.


Remove duplicate comments.

dborovcanin · 2024-12-09T11:50:05Z

service.go

+// using the provided template and the generated private key.
+// The certificate is then stored in the repository using the CreateCert method.
+// If the root CA is not found, it returns an error.
+func (s *service) IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, options SubjectOptions, key crypto.PrivateKey) (Certificate, error) {


Let's extract issuing certs to unexported method:

(s *service) issue(ctx context.Context, entityID, ttl string, ipAddrs []string, options SubjectOptions, pubKey crypto.PublicKey, privKey crypto.PrivateKey) (Certificate, error)

you can call from both IssuseCert and IssueFromCSR so that you can make the interface cleaner:

IssueCert(ctx context.Context, entityID, ttl string, ipAddrs []string, option SubjectOptions) (Certificate, error)

So we do not have any key params when issuing certs (so you don't have to use nil as the indicator you want to create a key pair).

Signed-off-by: nyagamunene <[email protected]>

dborovcanin · 2024-12-09T15:20:17Z

service.go

 	serialNumber, err := rand.Int(rand.Reader, serialNumberLimit)
 	if err != nil {
 		return Certificate{}, err
 	}

-	if s.intermediateCA.Certificate == nil || s.intermediateCA.PrivateKey == nil {
-		return Certificate{}, ErrIntermediateCANotFound
+	subject := s.getSubject(options)


Rename to subjectFromOpts. Also, no need to make it a service method, it's just an unexported function: func subjectFromOpts(opts SubjectOptions) pkix.Name.

Signed-off-by: nyagamunene <[email protected]>

nyagamunene added 10 commits November 28, 2024 19:29

Refactor CSR generation

eba2f56

Signed-off-by: nyagamunene <[email protected]>

failing linter

290d256

Signed-off-by: nyagamunene <[email protected]>

handle multiple private key types

459ffb3

Signed-off-by: nyagamunene <[email protected]>

fix failing linter

67e51e2

Signed-off-by: nyagamunene <[email protected]>

fix failing linter

cd6f2fd

Signed-off-by: nyagamunene <[email protected]>

fix parsing in transport layer

c795d21

Signed-off-by: nyagamunene <[email protected]>

Remove logging

5374de7

Signed-off-by: nyagamunene <[email protected]>

add multiple key type support

8588a8f

Signed-off-by: nyagamunene <[email protected]>

update issue cert method

a3fcde7

Signed-off-by: nyagamunene <[email protected]>

make the strings to constants

670bc52

Signed-off-by: nyagamunene <[email protected]>

nyagamunene marked this pull request as ready for review December 2, 2024 08:31

SammyOina requested changes Dec 2, 2024

View reviewed changes

rename signcsr to issuefromcsr

5ccc6f5

Signed-off-by: nyagamunene <[email protected]>

nyagamunene self-assigned this Dec 2, 2024

SammyOina requested changes Dec 2, 2024

View reviewed changes

nyagamunene added 2 commits December 2, 2024 15:39

move create csr to cli tool

f21503f

Signed-off-by: nyagamunene <[email protected]>

fix failing linter

2421ea4

Signed-off-by: nyagamunene <[email protected]>

SammyOina requested changes Dec 2, 2024

View reviewed changes

nyagamunene added 3 commits December 2, 2024 19:41

refactor issue cert method

2dbf045

Signed-off-by: nyagamunene <[email protected]>

fix failing linter

44e5e41

Signed-off-by: nyagamunene <[email protected]>

fix failing linter

01eb0bb

Signed-off-by: nyagamunene <[email protected]>

SammyOina previously approved these changes Dec 3, 2024

View reviewed changes

dborovcanin requested changes Dec 6, 2024

View reviewed changes

address comments

c412393

Signed-off-by: nyagamunene <[email protected]>

nyagamunene dismissed SammyOina’s stale review via c412393 December 8, 2024 18:02

dborovcanin requested changes Dec 9, 2024

View reviewed changes

change variable from any to crypto.PrivateKey

024409c

Signed-off-by: nyagamunene <[email protected]>

dborovcanin requested changes Dec 9, 2024

View reviewed changes

nyagamunene added 2 commits December 9, 2024 13:47

Refactor issueCerts

98275e7

Signed-off-by: nyagamunene <[email protected]>

fix failing linter

85dedd2

Signed-off-by: nyagamunene <[email protected]>

dborovcanin requested changes Dec 9, 2024

View reviewed changes

refactor issueCerts and issueforcsr

11972ae

Signed-off-by: nyagamunene <[email protected]>

dborovcanin requested changes Dec 9, 2024

View reviewed changes

rename getsubject to subjectFromOpts

86ceb56

Signed-off-by: nyagamunene <[email protected]>

dborovcanin approved these changes Dec 9, 2024

View reviewed changes

dborovcanin merged commit 04dbd2c into absmach:main Dec 9, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NOISSUE - Refactor CSR generation #55

NOISSUE - Refactor CSR generation #55

nyagamunene commented Nov 28, 2024 •

edited

Loading

SammyOina left a comment

SammyOina Dec 2, 2024

nyagamunene Dec 2, 2024

SammyOina Dec 2, 2024

SammyOina Dec 2, 2024

SammyOina Dec 2, 2024

nyagamunene Dec 2, 2024

dborovcanin Dec 6, 2024

dborovcanin Dec 6, 2024 •

edited

Loading

dborovcanin Dec 6, 2024

dborovcanin Dec 6, 2024

dborovcanin Dec 6, 2024

dborovcanin Dec 6, 2024

dborovcanin Dec 9, 2024

dborovcanin Dec 9, 2024

dborovcanin Dec 9, 2024

nyagamunene Dec 9, 2024

dborovcanin Dec 9, 2024

dborovcanin Dec 9, 2024

dborovcanin Dec 9, 2024

NOISSUE - Refactor CSR generation #55

NOISSUE - Refactor CSR generation #55

Conversation

nyagamunene commented Nov 28, 2024 • edited Loading

What type of PR is this?

What does this do?

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

Have you included tests for your changes?

Did you document any new/modified features?

Notes

SammyOina 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

Choose a reason for hiding this comment

dborovcanin Dec 6, 2024 • edited Loading

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

nyagamunene commented Nov 28, 2024 •

edited

Loading

dborovcanin Dec 6, 2024 •

edited

Loading