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

MG-2080 - Add TLS Certs Loading and Certificate Revocation #57

Merged
merged 43 commits into from
Apr 4, 2024

Conversation

arvindh123
Copy link
Contributor

@arvindh123 arvindh123 commented Feb 28, 2024

Pull request title should be MF-XXX - description or NOISSUE - description where XXX is ID of issue that this PR relate to.
Please review the CONTRIBUTING.md file for detailed contributing guidelines.

What does this do?

Add TLS Certs Loading and Certificate Revocation

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

Resolves https://github.com/absmach/magistrala/issues/2080.

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

curl -sSiX POST "${protocol}://${host}:${port}/${path}" -H "content-type:${content}" -H "Authorization:TOKEN" -d "${message}" --cacert $cafile 2>&1

echo -e "\nPosting message to ${protocol}://${host}:${port}/${path} with tls, Authorization header, & without ca , client certificates.."
curl -sSiX POST "${protocol}://${host}:${port}/${path}" -H "content-type:${content}" -H "Authorization:TOKEN" -d "${message}" 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.

Please update Readme with details on how to use examples and create relevant certificates using Makefile.

config.go Outdated
}

func (c *Config) EnvParse(opts env.Options) error {
if len(opts.FuncMap) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use switch instead of if-else.

pkg/tls/tls.go Outdated
Comment on lines 113 to 120
ClientCertValidationMethods []ValidateMethod `env:"CLIENT_CERT_VALIDATION_METHODS" envDefault:""`
OCSPDepth uint `env:"OCSP_DEPTH" envDefault:"0"`
OCSPResponderURL url.URL `env:"OCSP_RESPONDER_URL" envDefault:""`
CRLDepth uint `env:"CRL_DEPTH" envDefault:"1"`
OfflineCRLFile string `env:"OFFLINE_CRL_FILE" envDefault:""`
CRLDistributionPoints url.URL `env:"CRL_DISTRIBUTION_POINTS" envDefault:""`
CRLDistributionPointsSignCheck bool `env:"CRL_DISTRIBUTION_POINTS_SIGN_CHECK" envDefault:"false"`
CRLDistributionPointsIssuerCertFile string `env:"CRL_DISTRIBUTION_POINTS_ISSUER_CERT_FILE " envDefault:""`
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract these (revocation config) to a separate struct in a separate file.

pkg/tls/tls.go Outdated
}

// Load return a TLS configuration that can be used in TLS servers.
func (c *Config) Load() (*tls.Config, Security, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not return Security here. Returning nil, nil to indicate no config and no error seems a little cryptic (while correct, so I would suggest using the ErrNoTLS error to indicate no error, but also no TLS; so you can later have something like:

	l, err := net.Listen("tcp", p.config.Address)
	if err != nil {
		return err
	}
	tlsCfg, err := p.config.TLSConfig.Load()
	switch err {
	case nil:
		l = tls.NewListener(l, tlsCfg)
	case mptls.ErrNoTLS:
		break
	default:
		return err
	}

While this means we first start the listener and then parse the TLS config, this order is not so important; we can't have one without the other anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also need to know config is mTLS or only TLS , How can we find that config is TLS or mTLS ?

pkg/tls/tls.go Outdated
return []byte{}, nil
}

func (c *Config) verifyPeerCertificate(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this alongside the new file for revocation config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this method looks very complex. Can you try to simplify or, at least, split?

pkg/tls/tls.go Outdated
if !isRootCA(peerCertificate) {
if issuerCert == nil {
if len(peerCertificate.IssuingCertificateURL) < 1 {
return fmt.Errorf("neither the issuer certificate is present in the chain nor is the issuer certificate URL present in AIA, certificate common name %s and serial number %x", peerCertificate.Subject.CommonName, peerCertificate.SerialNumber)
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract large strings to constants. Try to simplify the method.

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.

Replace if-else with switch.

pkg/tls/tls.go Outdated
Comment on lines 24 to 46
type Security int

const (
WithoutTLS Security = iota + 1
WithTLS
WithmTLS
WithmTLSVerify
)

func (s Security) String() string {
switch s {
case WithTLS:
return "with TLS"
case WithmTLS:
return "with mTLS"
case WithmTLSVerify:
return "with mTLS and validation of client certificate revocation status"
case WithoutTLS:
fallthrough
default:
return "without TLS"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this whole construct and replace it with:

func Status(c Config) string {
	if c.CertFile == "" && c.KeyFile == "" {
		return "TLS"
	}

	if c.ClientCAFile != "" {
		if len(c.ClientValidation.ValidationMethods) > 0 {
			return "mTLS with client verification"
		}
		return "mTLS"
	}

	return "no TLS"
}

In main.go we have:

p.logger.Info(fmt.Sprintf("HTTP proxy server at %s%s with %s exiting with errors", p.config.Address, p.config.PrefixPath, mptls.Status(p.config.TLSConfig)), slog.String("error", err.Error()))

@@ -0,0 +1,434 @@
// Copyright (c) Abstract Machines
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to verifier. Also, separate OCSP and CRL into subpackages.

Signed-off-by: Arvindh <[email protected]>
config.go Outdated
Comment on lines 22 to 34
func (c *Config) EnvParse(opts env.Options) error {
switch {
case len(opts.FuncMap) == 0:
opts.FuncMap = map[reflect.Type]env.ParserFunc{
reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods): parseSliceValidateMethod,
reflect.TypeOf(verifier.OCSP): parseValidateMethod,
}
default:
opts.FuncMap[reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods)] = parseSliceValidateMethod
opts.FuncMap[reflect.TypeOf(verifier.OCSP)] = parseValidateMethod
}
return env.ParseWithOptions(c, opts)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify:

Suggested change
func (c *Config) EnvParse(opts env.Options) error {
switch {
case len(opts.FuncMap) == 0:
opts.FuncMap = map[reflect.Type]env.ParserFunc{
reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods): parseSliceValidateMethod,
reflect.TypeOf(verifier.OCSP): parseValidateMethod,
}
default:
opts.FuncMap[reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods)] = parseSliceValidateMethod
opts.FuncMap[reflect.TypeOf(verifier.OCSP)] = parseValidateMethod
}
return env.ParseWithOptions(c, opts)
}
func (c *Config) EnvParse(opts env.Options) error {
if opts.FuncMap == nil {
opts.FuncMap = make(map[reflect.Type]env.ParserFunc)
}
opts.FuncMap[reflect.TypeOf(c.TLSConfig.Verifier.ValidationMethods)] = parseSliceValidateMethod
opts.FuncMap[reflect.TypeOf(verifier.OCSP)] = parseValidateMethod
return env.ParseWithOptions(c, opts)
}

pkg/tls/tls.go Outdated
return tlsConfig, nil
}

func (c *Config) Security() Security {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use this as a util function, do not use Config receiver. Also, move it to a different package.

pkg/tls/tls.go Outdated
Verifier verifier.Config
}

func (c *Config) SecurityStatus() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, remove; use util method.

pkg/tls/tls.go Outdated
// LoadTLSCfg return a TLS configuration that can be used in TLS servers.
func LoadTLSCfg(ca, crt, key string) (*tls.Config, error) {
caCertPEM, err := os.ReadFile(ca)
func (s Security) String() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove, we do not need a new type; use the util method to determine log levels.

pkg/tls/tls.go Outdated
KeyFile string `env:"KEY_FILE" envDefault:""`
ServerCAFile string `env:"SERVER_CA_FILE" envDefault:""`
ClientCAFile string `env:"CLIENT_CA_FILE" envDefault:""`
Verifier verifier.Config
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with the list of verifiers, where Verifier is:

type Verifier interface {
	// VerifyPeerCertificate...
	VerifyPeerCertificate(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error
}

and in the Config we have:

func (c *Config) VerifyPeerCertificate(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
	for _, v := range c.Verifiers {
		if err := v.VerifyPeerCertificate(rawCerts, verifiedChains); err != nil {
			return err
		}
	}
	return nil
}

And you use different implementations of Verifier ocsp and crl just like you already did.

pkg/tls/tls.go Outdated
}
tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert
if len(c.Verifier.ValidationMethods) > 0 {
tlsConfig.VerifyPeerCertificate = c.Verifier.VerifyPeerCertificate
Copy link
Contributor

Choose a reason for hiding this comment

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

And here it would be something like:

		tlsConfig.VerifyPeerCertificate = c.VerifyPeerCertificate

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.

Please fix CI liner comments.

Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
@dborovcanin dborovcanin changed the title NOISSUE - Add TLS Certs Loading and Certificate Revocation MG-2080 - Add TLS Certs Loading and Certificate Revocation Apr 3, 2024
dborovcanin and others added 3 commits April 3, 2024 13:26
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
NOISSUE - Refactor TLS layer
@arvindh123 arvindh123 requested a review from dborovcanin April 3, 2024 15:59
README.md Outdated
Comment on lines 184 to 199
- Script to test mProxy server running at port 1884 for MQTT without TLS

```bash
examples/client/mqtt/without_tls.sh
```

- Script to test mProxy server running at port 8883 for MQTT with TLS

```bash
examples/client/mqtt/with_tls.sh
```

- Script to test mProxy server running at port 8884 for MQTT with mTLS

```bash
examples/client/mqtt/with_mtls.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix examples/client/http/....

Signed-off-by: Arvindh <[email protected]>
@@ -0,0 +1,3 @@
listener 1883
listener 8000
protocol websockets
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add allow_anonymous true to allow using more recent Mosquitto versions.

@dborovcanin dborovcanin merged commit d3495b1 into absmach:main Apr 4, 2024
1 check passed
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.

Check if NginX Lua script is still necessary for mTLS
2 participants