Implement bestEffort functions for hashable and equatable. #280

madsodgaard · 2021-03-13T13:43:33Z

This PR adds bestEffortHash and bestEffortEquals in order to provide best effort method for using Hashable and Equatable in other projects. This is useful/needed to solve a problem such as swift-server/async-http-client#330

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-13T13:43:35Z

Can one of the admins verify this patch?

weissi

Thank you. That’s a good start! We’ll need to fix the equals method and need some tests too.

Sources/NIOSSL/TLSConfiguration.swift

glbrntt · 2021-03-15T09:35:41Z

@swift-nio-bot test this please

Sources/NIOSSL/TLSConfiguration.swift

Lukasa

Hmm, I'm uncertain about whether it's a good idea to define equatability and hashability for this object in a way that does not include the key log callback. This seems likely to cause surprising behaviours in cases where the type is used as a key.

I'm inclined to suggest we should solve the problem in async-http-client a different way, rather than using this incomplete and surprising definition for equatability and hashability.

madsodgaard · 2021-03-15T12:06:55Z

What are the alternative ways of solving this problem? If TLSConfiguration is not Hashable we can't use it in our key for the connection pool, if I understand correctly? I'd be happy to try another way, just not sure what that way could be, other than not utilizing the pool.

Lukasa · 2021-03-15T15:32:24Z

Use something that isn't TLSConfiguration but that either can be derived from it or that can derive it.

In this case it seems like if we wanted to do the least effort possible we could just put these definitions on a wrapper structure whose equality is defined this way. However, a better move is probably to have AHC have its own notion of TLS configuration not directly tied to swift-nio-ssl's, not least given that it works with multiple TLS backends anyway.

weissi · 2021-03-16T11:00:53Z

@Lukasa / @madsodgaard I agree that we should only hash in here if we can include all callbacks, we definitely can't just compare everything but the callbacks. If the callbacks are all C callbacks, then I'm happy comparing their pointer address. Cory wdyt?

@madsodgaard what Cory suggests is what I meant with if we can't reasonably hash it in here, then we can hack around it in AHC. The simplest way (in AHC) is struct TLSConfigurationHashableHack: Hashable { var tlsConfigureation: TLSConfiguration }. Then you can define hashable on TLSConfigurationHashableHack.

madsodgaard · 2021-03-16T11:57:17Z

@weissi Oh okay. And then it would be fine that we don't include the callbacks in AHC's definition of what Hashable is for TLSConfiguration(Wrapper)?

weissi · 2021-03-16T12:09:44Z

@madsodgaard tbh, I don't think we can just not hash the callbacks at all. There can conceivably two distinct TLSConfiguration that only differ in the callbacks. And the callbacks are very meaningful because that's where the user tells us whether to trust a cert or not. It would be really bad if the user configured an "always accept" callback that would then get reused for something else.

Lukasa · 2021-03-16T13:35:06Z

The callbacks are not C callbacks, they're Swift callbacks.

weissi · 2021-03-16T21:00:36Z

The callbacks are not C callbacks, they're Swift callbacks.

@Lukasa / @madsodgaard Okay, then we have basically one option: hash/compare the bytes of the closure

withUnsafeBytes(of: &self.theClosure) { closureBits in
    var hasher = Hasher()
    closureBits.forEach { hasher.combine($0) }
}

@Lukasa wdyt?

Lukasa · 2021-03-17T09:52:39Z

I remain uncertain of why we think we have to do this, for two reasons.

Firstly, AHC has already made the mistake of trying to support two TLS implementations using the configuration for only one of them. This is a bad decision, and we shouldn't propagate it further. AHC should have a general TLS configuration object that it can use to derive the specific ones, and that object absolutely can be hashable.

Secondly, even if we disregarded that, there is only one callback on TLSConfiguration and it's keyLogCallback, used to implement SSLKEYLOGFILE support. Do we really need to drop into unsafe code to add hashability to this type for that?

weissi · 2021-03-17T10:45:51Z

I remain uncertain of why we think we have to do this, for two reasons.

The upsides of doing it in NIOSSL are:

NIOSSL knows exactly what's part of TLSConfiguration (in the latest version), it's harder to forget to add a newly added member to the equals/hashable implementations
NIOSSL has to deal with unsafe code already so arguably it's the best place to do it.

One other option of doing it inside NIOSSL is to instead of implementing Hashable, exporting two functions:

extension TLSConfiguration {
    // will say no to two unequals, tries to say yes to two equals but can't guarantee no false negatives
    public func bestEffortIsEqualTo(_ other: TLSConfiguration) -> Bool {
       ...
    }

    // two objects that are equal wrt `bestEffortIsEqualTo` are guaranteed to yield the same hash, others do not.
    public func bestEffortHash(into: inout Hasher) -> UInt64 {
        ...
    }
}

WDYT?

Firstly, AHC has already made the mistake of trying to support two TLS implementations using the configuration for only one of them. This is a bad decision, and we shouldn't propagate it further. AHC should have a general TLS configuration object that it can use to derive the specific ones, and that object absolutely can be hashable.

Secondly, even if we disregarded that, there is only one callback on TLSConfiguration and it's keyLogCallback, used to implement SSLKEYLOGFILE support. Do we really need to drop into unsafe code to add hashability to this type for that?

Fair enough, @madsodgaard in that case I'd suggest to move this code into an internal type into AHC which does

internal struct HashableTLSConfiguration {
   var tlsConfiguration: TLSConfiguration
}

extension HashableTLSConfiguration: Hashable { 
   ...
}

and then inside AHC, we'd just put a HashableTLSConfiguration into the connection pool instead of a TLSConfiguration.

If @Lukasa thinks the bestEffortIsEqualTo/HashCode here is a good idea, then we still need those types in AHC but they'd just need to call into the best effort functions.

weissi · 2021-03-20T18:40:16Z

friendly ping @Lukasa on the bestEffort* in this repo

Lukasa · 2021-03-22T09:18:20Z

I'm not sure that we gain much from the bestEffort implementations tbqh.

weissi · 2021-03-23T10:07:42Z

I'm not sure that we gain much from the bestEffort implementations tbqh.

But how are the dependencies supposed to hash/compare the right things? We'll add things to the TLS config in the future and then things will start to subtle break. So I think NIOSSL can definitely provide the best fidelity here (and IMHO should). But I agree with you that we shouldn't do it with Hashable because it's only a best effort.

Lukasa · 2021-03-23T10:08:39Z

The question remains why dependencies are hashing the TLS configuration at all.

weissi · 2021-03-23T10:47:19Z

The question remains why dependencies are hashing the TLS configuration at all.

For connection pools, we need to be able to compare the TLS connection of the connection in the pool with the TLS connection that the user would like for the request. False negatives are fine, we can always create a new connection.

That means we need (best effort) comparison (so the user can create an internal type wrapping the TLS config that adds Equatable). And because it's kinda nice to store the pool in a dictionary so they'd need to make it hashable.

Lukasa · 2021-03-23T11:28:57Z

Yes, I understand the proposed design. My question is whether that proposed design is good.

Specifically, should libraries encapsulating swift-nio-ssl pervasively expose TLSConfiguration as the mechanism by which they configure TLS. My general answer is no, they should not, because it couples them tightly to their TLS implementation. If users instead provide an encapsulating general-purpose TLS configuration that they use to generate a swift-nio-ssl TLSConfiguration object, only the general-purpose one needs to be Hashable. That seems to me to be a vastly preferable design, no?

weissi · 2021-03-23T12:09:18Z

Specifically, should libraries encapsulating swift-nio-ssl pervasively expose TLSConfiguration as the mechanism by which they configure TLS. My general answer is no, they should not, because it couples them tightly to their TLS implementation.

I see! I agree with you here.

That seems to me to be a vastly preferable design, no?

Absolutely. I do however think that evangelism and not preventing the best effort eq/hash APIs is the best way to get there. Fact is that there are libraries that use NIOSSL.TLSConfiguration today and I think we should allow them (through the best effort eq/hash APIs) to solve their problem today. I'm happy with adding a - warning: If you need this, then you are probably doing ... but shouldn't.

This is compounded by the fact that creating a TLS configuration object that can then yield a NIOSSL.TLSConfiguration/Security.TLSOptions/Windows equivalent isn't actually super easy and there's (AFAIK) no prior art in Swift. So not offering the proposed APIs leaves the authors of those libraries in a tough spot.

Short of now offering connection pools, they only really replicate the eq/hash functionality in their own packages which I think may be at a risk of security vulnerabilities because we may add properties that they miss. That could make their hand-rolled eq/hash functionality have false positives (claiming two TLSConfigs are equal when they're not) and that's bad.

Lukasa · 2021-03-23T15:34:28Z

I think if we think we should offer this, then we should go whole hog and use withUnsafeBytes(of:) the closure.

weissi · 2021-03-23T16:27:02Z

I think if we think we should offer this, then we should go whole hog and use withUnsafeBytes(of:) the closure.

~~Works for me. I'm mildly worried about ASan complaining that we read bytes that were never written (padding) but I'm totally happy deferring that to when/if we actually hit this issue.~~

Sorry, ignore the above. I'm happy with this. I thought you meant withUnsafeBytes(of:) the whole TLSConfiguration.

madsodgaard · 2021-04-26T15:30:17Z

@Lukasa @weissi I implemented the bestEffort* functions from what I understood from your conclusion. I added some documentation/warnings, but let me know what you think is appropriate to put there!

Tests/NIOSSLTests/TLSConfigurationTest.swift

weissi · 2021-04-26T15:55:11Z

Sources/NIOSSL/TLSConfiguration.swift

+    ///
+    /// - warning: You should probably not use this function. This function can return false-negatives, but not false-positives.
+    public func bestEffortEquals(_ comparing: TLSConfiguration) -> Bool {
+        let isKeyLoggerCallbacksEqual = withUnsafeBytes(of: self.keyLogCallback) { callbackPointer1 in


given SR-14520 we should probably make this

let isBitEqual = withUnsafeBytes(of: self) { selfBits in withUnsafeBytes(of: comparing) { otherBits in return selfBits.elementsEqual(otherBits) } }

with a comment liking the bug.

@Lukasa wdyt?

I still think we should stick with the closure-based version, but yes agree we should link the bug.

@Lukasa the problem is that if we do hit the bug, then they will never be equal. The allocations are for reabstraction thunks which aren't equal because they capture (the heap allocs) :P

weissi

Thank you! That looks good to me!

weissi · 2021-05-03T12:18:14Z

@swift-nio-bot add to allowlist

Lukasa

Can you run the generate_linux_tests.rb script? That's part of the issues we're seeing in CI: we're missing your new tests on Linux.

madsodgaard · 2021-05-04T17:26:44Z

@Lukasa Done!

weissi · 2021-05-05T08:27:20Z

@Lukasa this is now all green, the failure is the new 5.5 job which doesn't have docker images yet ;)

Make TLSConfiguration Hashable

a69c141

weissi requested changes Mar 13, 2021

View reviewed changes

Sources/NIOSSL/TLSConfiguration.swift Outdated Show resolved Hide resolved

weissi requested a review from Lukasa March 13, 2021 15:07

madsodgaard added 3 commits March 13, 2021 16:22

Fix equatable conformance

e521df8

Add basic tests

6bbacec

Remove hash not equal check

fbf53b5

madsodgaard requested a review from weissi March 13, 2021 19:00

glbrntt reviewed Mar 15, 2021

View reviewed changes

Sources/NIOSSL/TLSConfiguration.swift Outdated Show resolved Hide resolved

glbrntt added the 🆕 semver/minor Adds new public API. label Mar 15, 2021

Lukasa reviewed Mar 15, 2021

View reviewed changes

Implement bestEffort functions

c884cac

madsodgaard changed the title ~~Make TLSConfiguration Hashable~~ Implement bestEffort functions for hashable and equatable. Apr 24, 2021

add doc

ae95286

weissi reviewed Apr 26, 2021

View reviewed changes

add test for different callbacks

860c172

madsodgaard mentioned this pull request Apr 30, 2021

Support request specific TLS configuration swift-server/async-http-client#358

Merged

weissi requested a review from Lukasa May 3, 2021 12:15

weissi approved these changes May 3, 2021

View reviewed changes

Lukasa requested changes May 4, 2021

View reviewed changes

generate tests

f0701a7

Merge branch 'main' into mo-tls-hashable

ff4ad7a

weissi requested a review from Lukasa May 4, 2021 20:26

Merge branch 'main' into mo-tls-hashable

1fbcac8

Lukasa approved these changes May 5, 2021

View reviewed changes

Lukasa merged commit b2f8f35 into apple:main May 5, 2021

Implement bestEffort functions for hashable and equatable. #280

Implement bestEffort functions for hashable and equatable. #280

Uh oh!

Conversation

madsodgaard commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

swift-server-bot commented Mar 13, 2021

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glbrntt commented Mar 15, 2021

Uh oh!

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

madsodgaard commented Mar 15, 2021

Uh oh!

Lukasa commented Mar 15, 2021

Uh oh!

weissi commented Mar 16, 2021

Uh oh!

madsodgaard commented Mar 16, 2021

Uh oh!

weissi commented Mar 16, 2021

Uh oh!

Lukasa commented Mar 16, 2021

Uh oh!

weissi commented Mar 16, 2021

Uh oh!

Lukasa commented Mar 17, 2021

Uh oh!

weissi commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weissi commented Mar 20, 2021

Uh oh!

Lukasa commented Mar 22, 2021

Uh oh!

weissi commented Mar 23, 2021

Uh oh!

Lukasa commented Mar 23, 2021

Uh oh!

weissi commented Mar 23, 2021

Uh oh!

Lukasa commented Mar 23, 2021

Uh oh!

weissi commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lukasa commented Mar 23, 2021

Uh oh!

weissi commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madsodgaard commented Mar 13, 2021 •

edited

Loading

weissi commented Mar 17, 2021 •

edited

Loading

weissi commented Mar 23, 2021 •

edited

Loading

weissi commented Mar 23, 2021 •

edited

Loading