NOISSUE - Refactor CSR generation by nyagamunene · Pull Request #55 · absmach/certs

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 <stevenyaga2014@gmail.com>

SammyOina

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

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

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

 	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 <stevenyaga2014@gmail.com>

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

 		}

-		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 <stevenyaga2014@gmail.com>

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

+	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

+		}
+		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

+		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

+		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

+	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

+		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 <stevenyaga2014@gmail.com>

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

+// 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


 	// 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 <stevenyaga2014@gmail.com>

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


 	// 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 <stevenyaga2014@gmail.com>

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

+// 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

+// 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 <stevenyaga2014@gmail.com>

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


-	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 <stevenyaga2014@gmail.com>

nyagamunene added 10 commits November 28, 2024 19:29

Refactor CSR generation

eba2f56

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

failing linter

290d256

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

handle multiple private key types

459ffb3

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

fix failing linter

67e51e2

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

fix failing linter

cd6f2fd

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

fix parsing in transport layer

c795d21

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

Remove logging

5374de7

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

add multiple key type support

8588a8f

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

update issue cert method

a3fcde7

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

make the strings to constants

670bc52

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

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

SammyOina suggested changes Dec 2, 2024

View reviewed changes

rename signcsr to issuefromcsr

5ccc6f5

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

nyagamunene self-assigned this Dec 2, 2024

SammyOina suggested 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 <stevenyaga2014@gmail.com>

fix failing linter

2421ea4

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

SammyOina suggested 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 <stevenyaga2014@gmail.com>

fix failing linter

44e5e41

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

fix failing linter

01eb0bb

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

SammyOina previously approved these changes Dec 3, 2024

View reviewed changes

dborovcanin suggested changes Dec 6, 2024

View reviewed changes

address comments

c412393

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

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

dborovcanin suggested changes Dec 9, 2024

View reviewed changes

change variable from any to crypto.PrivateKey

024409c

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

dborovcanin suggested changes Dec 9, 2024

View reviewed changes

nyagamunene added 2 commits December 9, 2024 13:47

Refactor issueCerts

98275e7

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

fix failing linter

85dedd2

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

dborovcanin suggested changes Dec 9, 2024

View reviewed changes

refactor issueCerts and issueforcsr

11972ae

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

dborovcanin suggested changes Dec 9, 2024

View reviewed changes

rename getsubject to subjectFromOpts

86ceb56

Signed-off-by: nyagamunene <stevenyaga2014@gmail.com>

dborovcanin approved these changes Dec 9, 2024

View reviewed changes

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

Uh oh!

Conversation

nyagamunene commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

SammyOina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dborovcanin Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nyagamunene commented Nov 28, 2024 •

edited

Loading

dborovcanin Dec 6, 2024 •

edited

Loading