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

Bring us closer to crypto/tls #148

Closed
daenney opened this issue Nov 12, 2019 · 8 comments
Closed

Bring us closer to crypto/tls #148

daenney opened this issue Nov 12, 2019 · 8 comments
Labels

Comments

@daenney
Copy link
Member

daenney commented Nov 12, 2019

As part of v2 I'd like us to take some time and see if there's more of crypto/tls we can reuse. Mostly want to ensure we don't diverge from interafces and behaviour in crypto/tls unless mandated by the DTLS RFCs. This should help newcomers to immediately get started with pion/dtls as it's hopefully just swapping out crypto/tls for pion/dtls.

@daenney daenney added the v2 label Nov 12, 2019
@daenney daenney mentioned this issue Nov 12, 2019
4 tasks
@at-wat
Copy link
Member

at-wat commented Feb 1, 2020

I have take a look around crypto/tls.
Followings are my suggestion to make pion/dtls closer to crypto/tls

Unexpose or move non-DTLS things

type Closer (#183) ✔️

It is a package internal utility. It would be better to be unexposed.

func GenerateSelfSigned(), func SelfSign() (#183) ✔️

They are for testing and not DTLS specific. It would be better to be unexposed or moved it to somewhere.

func Fingerprint() (#185) ✔️

It looks be a part of SDP more than DTLS. Moving it to pion/sdp would be better.
Not a part of DTLS. (not used in dtls package.)
It is currently used by:

webrtc/quictransport.go:		remoteValue, err := dtls.Fingerprint(remoteCert, hashAlgo)
webrtc/certificate.go:		value, err := dtls.Fingerprint(c.x509Cert, algo)
webrtc/dtlstransport.go:		remoteValue, err := dtls.Fingerprint(remoteCert, hashAlgo)

Interfacing

type Listener (#181) ✔️

type Listener would better be implement net.Listener.
Close(shutdownTimeout time.Duration) differs from net.Listener. Putting shutdownTimeout in Config struct looks better.

Misc.

Clean-up connection timeout config interface

In crypto/tls, TCP connection timeout is configured via Timeout field in net.Dialer. I think adding dtls.DialWithDialer() which is compatible with tls.DialWithDialer() and making default dialer having 30 seconds of default timeout would be clean and good at compatibility with crypto/tls.
It looks unsuitable at now as the package exports dtls.Server and dtls.Client, and DTLS requires timeout also in Server side inside Listener.Accpt() which don't receive Dialer.

@at-wat
Copy link
Member

at-wat commented Feb 2, 2020

func Fingerprint()

Maybe better to move Fingerprint and also HashAlgorithm under pkg/crypto/?


ID of the hash algorithm is TLS specific.

dtls/hash_algorithm.go

Lines 11 to 13 in 0b80bd9

// HashAlgorithm is used to indicate the hash algorithm used
// https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-18
type HashAlgorithm uint16

As a result of looking around the code, I think public APIs would be better to receive crypro.Hash and internally translate to hash ID.

@at-wat
Copy link
Member

at-wat commented Feb 3, 2020

Behavior

type Listener (#181) ✔️

  • Accepted connections should be available after Listener.Close().

type Conn (#184) ✔️

Pass nettest.ConnTest.

  • Read should return already buffered data even after the connection is closed.
  • SetDeadline should be implemented.

@at-wat
Copy link
Member

at-wat commented Feb 3, 2020

@daenney @rumpelsepp @jkralik @Sean-Der I have created some PRs to make public APIs more closer to crypto/tls. Please add comments if you have any ideas on them.

@daenney
Copy link
Member Author

daenney commented Feb 7, 2020

I noticed something in the Go 1.14 releases:

  • The new CipherSuites and InsecureCipherSuites functions return a list of currently implemented cipher suites
  • The new CipherSuiteName function returns a name for a cipher suite ID

Might be nice if we provide these too. CipherSuiteName() can be implemented by calling dtls.cipherSuite.String(), since we already have that one.

CipherSuites and InsecureCipherSuites should return a []tls.CipherSuite which is a bit different from our dtls.cipherSuite. It should be easy to map to those though:

type CipherSuite struct {
    ID   uint16
    Name string

    // Supported versions is the list of TLS protocol versions that can
    // negotiate this cipher suite.
    SupportedVersions []uint16

    // Insecure is true if the cipher suite has known security issues
    // due to its primitives, design, or implementation.
    Insecure bool
}
type cipherSuite interface {
	String() string
	ID() CipherSuiteID
	certificateType() clientCertificateType
	hashFunc() func() hash.Hash
	isPSK() bool
	isInitialized() bool

	// Generate the internal encryption state
	init(masterSecret, clientRandom, serverRandom []byte, isClient bool) error

	encrypt(pkt *recordLayer, raw []byte) ([]byte, error)
	decrypt(in []byte) ([]byte, error)
}

daenney added a commit that referenced this issue Feb 8, 2020
This adds the CiperSuiteName function. It behaves exactly like
the tls.CipherSuiteName function that's being added for Go 1.14.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
As part of Go 1.14 the CipherSuites and InsecureCipherSuites functions
got added to the TLS package, returning a slice of *tls.CipherSuite.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
As part of Go 1.14 the CipherSuites and InsecureCipherSuites functions
got added to the TLS package, returning a slice of *tls.CipherSuite.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
This adds the CiperSuiteName function. It behaves exactly like
the tls.CipherSuiteName function that's being added for Go 1.14.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
As part of Go 1.14 the CipherSuites and InsecureCipherSuites functions
got added to the TLS package, returning a slice of *tls.CipherSuite.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
As part of Go 1.14 the CipherSuites and InsecureCipherSuites functions
got added to the TLS package, returning a slice of *tls.CipherSuite.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
As part of Go 1.14 the CipherSuites and InsecureCipherSuites functions
got added to the TLS package, returning a slice of *tls.CipherSuite.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
This adds the CiperSuiteName function. It behaves exactly like
the tls.CipherSuiteName function that's being added for Go 1.14.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
As part of Go 1.14 the CipherSuites and InsecureCipherSuites functions
got added to the TLS package, returning a slice of *tls.CipherSuite.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
This adds the CiperSuiteName function. It behaves exactly like
the tls.CipherSuiteName function that's being added for Go 1.14.

Relates to #148
daenney added a commit that referenced this issue Feb 8, 2020
As part of Go 1.14 the CipherSuites and InsecureCipherSuites functions
got added to the TLS package, returning a slice of *tls.CipherSuite.

Relates to #148
daenney added a commit that referenced this issue Feb 9, 2020
This adds the CiperSuiteName function. It behaves exactly like
the tls.CipherSuiteName function that's being added for Go 1.14.

Relates to #148
daenney added a commit that referenced this issue Feb 9, 2020
As part of Go 1.14 the CipherSuites and InsecureCipherSuites functions
got added to the TLS package, returning a slice of *tls.CipherSuite.

Relates to #148
@at-wat
Copy link
Member

at-wat commented Feb 10, 2020

I think this has been landed, isn't it?

@daenney
Copy link
Member Author

daenney commented Feb 10, 2020

Sure looks like it 🎉.

@at-wat
Copy link
Member

at-wat commented Feb 11, 2020

🎉

@at-wat at-wat closed this as completed Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants