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

fix: MarshalYAML receivers on TLSVersion and Curve #288

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mahendrapaipuri
Copy link

@mahendrapaipuri mahendrapaipuri commented Jan 28, 2025

Currently the MarshalYAML receivers are defined on pointers of TLSVersion and Curve but TLSConfig is defined with values of TLSVersion and Curve in its fields. So, MarshalYAML receiver is never called upon. This commit fixes it by changing receivers to non-pointer values which should work on both values and pointers.

Reproducer that shows the behaviour:

package main

import (
	"crypto/tls"
	"errors"
	"fmt"

	"github.com/prometheus/exporter-toolkit/web"
	"gopkg.in/yaml.v3"
)

type TLSVersion uint16

var tlsVersions = map[string]TLSVersion{
	"TLS13": (TLSVersion)(tls.VersionTLS13),
	"TLS12": (TLSVersion)(tls.VersionTLS12),
	"TLS11": (TLSVersion)(tls.VersionTLS11),
	"TLS10": (TLSVersion)(tls.VersionTLS10),
}

func (tv *TLSVersion) UnmarshalYAML(unmarshal func(interface{}) error) error {
	var s string
	err := unmarshal((*string)(&s))
	if err != nil {
		return err
	}
	if v, ok := tlsVersions[s]; ok {
		*tv = v
		return nil
	}
	return errors.New("unknown TLS version: " + s)
}

// NOTE THAT WE HAVE CHANGED RECEIVER TO NON-POINTER VALUE
func (tv TLSVersion) MarshalYAML() (interface{}, error) {
	for s, v := range tlsVersions {
		if tv == v {
			return s, nil
		}
	}
	return fmt.Sprintf("%v", tv), nil
}

type Curve tls.CurveID

var curves = map[string]Curve{
	"CurveP256": (Curve)(tls.CurveP256),
	"CurveP384": (Curve)(tls.CurveP384),
	"CurveP521": (Curve)(tls.CurveP521),
	"X25519":    (Curve)(tls.X25519),
}

func (c *Curve) UnmarshalYAML(unmarshal func(interface{}) error) error {
	var s string
	err := unmarshal((*string)(&s))
	if err != nil {
		return err
	}
	if curveid, ok := curves[s]; ok {
		*c = curveid
		return nil
	}
	return errors.New("unknown curve: " + s)
}

// NOTE THAT WE HAVE CHANGED RECEIVER TO NON-POINTER VALUE
func (c Curve) MarshalYAML() (interface{}, error) {
	for s, curveid := range curves {
		if c == curveid {
			return s, nil
		}
	}
	return fmt.Sprintf("%v", c), nil
}

type NewConfig struct {
	TLSConfig TLSConfig `yaml:"tls_server_config"`
}

type TLSConfig struct {
	MinVersion       TLSVersion `yaml:"min_version"`
	MaxVersion       TLSVersion `yaml:"max_version"`
	CurvePreferences []Curve    `yaml:"curve_preferences"`
}

func main() {
	actualConfig := web.Config{
		TLSConfig: web.TLSConfig{
			MinVersion:       web.TLSVersion(tls.VersionTLS12),
			MaxVersion:       web.TLSVersion(tls.VersionTLS13),
			CurvePreferences: []web.Curve{web.Curve(tls.CurveP256)},
			CipherSuites:     []web.Cipher{web.Cipher(tls.TLS_AES_128_GCM_SHA256)},
		},
	}

	newConfig := NewConfig{
		TLSConfig: TLSConfig{
			MinVersion:       TLSVersion(tls.VersionTLS12),
			MaxVersion:       TLSVersion(tls.VersionTLS13),
			CurvePreferences: []Curve{Curve(tls.CurveP256)},
		},
	}

	actualConfigYAML, _ := yaml.Marshal(&actualConfig)
	fmt.Println("Actual Config struct: \n", string(actualConfigYAML))

	newConfigYAML, _ := yaml.Marshal(&newConfig)
	fmt.Println("Modified Config struct: \n", string(newConfigYAML))
}

This should produce an output as follows:

Actual Config struct 
tls_server_config:
    cert: ""
    key: null
    client_ca: ""
    cert_file: ""
    key_file: ""
    client_auth_type: ""
    client_ca_file: ""
    cipher_suites:
        - TLS_AES_128_GCM_SHA256
    curve_preferences:
        - 23
    min_version: 771
    max_version: 772
    prefer_server_cipher_suites: false
    client_allowed_sans: []
http_server_config:
    http2: false
basic_auth_users: {}

Modified Config struct: 
tls_server_config:
    min_version: TLS12
    max_version: TLS13
    curve_preferences:
        - CurveP256

With current package's implementation, yaml.Marshal produces uint for min_version, max_version and curve_preferneces. When changed to receivers of MarshalYAML to non-pointer values, marshalled config produces correct output.

This PR fixes the marshall receivers and add unit tests to verify config file generation from structs. omitempty has been added to the config fields to avoid producing "zero" values when config file is being generated from Go structs. When fields are not set, the library use "sane" defaults which can be leveraged by client libraries.

* Currently the `MarshalYAML` receivers are defined on pointers of `TLSVersion` and `Curve` but `TLSConfig` is defined with values of `TLSVersion` and `Curve` in its fields. So, `MarshalYAML` receiver is never called upon. This commit fixes it by changing receivers on values which should work on both values and pointers.

Signed-off-by: Mahendra Paipuri <[email protected]>
@SuperQ
Copy link
Member

SuperQ commented Feb 6, 2025

Seems like we should have tests for this behavior.

@mahendrapaipuri
Copy link
Author

Cheers @SuperQ !! Currently there are no unit tests that generates YAML files from Go structs. Happy to add few unit tests for this use case.

@SuperQ
Copy link
Member

SuperQ commented Feb 6, 2025

Yes, if you could add some tests, that would be very useful.

* Add omitempty tags to all fields in the config to avoid rendering zero values in generated config

Signed-off-by: Mahendra Paipuri <[email protected]>
@mahendrapaipuri
Copy link
Author

@SuperQ Added few unit tests and updated PR description

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.

2 participants