-
Notifications
You must be signed in to change notification settings - Fork 229
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
[BUG] (Fake|Utls)PreSharedKeyExtension #248
Comments
Addtionally ... Noticed something odd happening when using |
Interesting observation. I don't believe we are ready to parse raw byte ClientHello into PSK yet. On the other hand it is theoretically not possible to resume the connection with just the PSK ticket and binder from raw bytes. That said, it might worth debugging and fixing to make at least FakePreSharedKeyExtension work with raw bytes. But note that in the end, unexpected things would happen, such as server rejecting you for submitting a valid ticket with an incorrect binder, etc. Tagging @3andne for visibility. |
To clarify on the intended behaviors:
|
Given all the complexities, I would encourage avoiding reusing raw bytes with PSK, either way it will not work as you expect, either we need to use a different PSK ticket/binder, or the connection will fail outright. |
FWIW, it use to work, prior to the change (when genericExtension 41 was used etc) |
Yep, and in that case, you would actually expect the server to return an error. Just based on my test result, when I use a legitimate PSK Identity with its original binder, the server gives me an error (due to failing the binder check). Have you actually observed any other behaviors? That would be really intriguing. And a quick patch here could be just roll back to use |
Yea i started doing that last night, tho ran into an issue with unable to decrypt error , trying to work out what is going on |
I don't remember exactly what error did I receive when I was testing. But there are essentially two possible outcomes:
|
This just gave me hint , when i was testing my custom spec i had grease place holder... so i'll test with static grease id's and let you know on that Switching back to rawBytes and using a genericExtenstion 41 here worked, the PSK loaded from raw bytes and was present on the hello Not sure why that's in a |
@gaukas what do you think about doing something with this and replace it with if ext != nil && ok { // known extension and implements TLSExtensionWriter properly
switch extension {
case extensionPreSharedKey:
// PSK extension, need to see if we do real or fake PSK
if realPSK {
extWriter = &UtlsPreSharedKeyExtension{}
} else {
if len(extData) == 0 {
extWriter = &FakePreSharedKeyExtension{}
} else {
extWriter = &GenericExtension{Id: 41}
}
}
case extensionSupportedVersions:
chs.TLSVersMin = 0
chs.TLSVersMax = 0
}
if _, err := extWriter.Write(extData); err != nil {
return err
}
chs.Extensions = append(chs.Extensions, extWriter) And adding a simple write to the GenericExtension in the tls_extensions func (e *GenericExtension) Write(b []byte) (int, error) {
e.Data = make([]byte, len(b))
n := copy(e.Data, b)
return n, nil
} So if the |
|
Ok, i'll dive deeper into |
Just an update on this, (sorry been busy), I can see something is happening when Sorry it appears before that, it Does a
Update:Found the "problem" with the disappearing PSK... its in if we remove that line PSK is populated... I need to read the code to understand the impact of removing it |
What if we did something like the following for that line if s.pskExtension.Len() != 0 || s.uconnRef.Extensions[i].Len() == 0 {
s.uconnRef.Extensions[i] = s.pskExtension
} |
It feels like that most likely you do not want to explicitly provide a PSK extension in case you are reading from raw bytes. So I would expect it to hit Line 273 instead. Lines 271 to 277 in df6e4c8
|
How do you feel about adding case *FakePreSharedKeyExtension:
if len(ext.Identities) != 0 && len(ext.Binders) != 0 {
uconn.sessionController.overridePskExt(ext)
} to here? Line 2486 in df6e4c8
Obviously that's slower as it goes and calls all the sub functions to pull the whole extension again.. but... Line 80 in df6e4c8
|
I'm not sure if that's the correct usage of overridePskExt()... I might need to looks into this a bit. |
other option is simply... if s.pskExtension == nil || ext.Len() != 0 { Line 271 in df6e4c8
THe problem is hence why i looked at |
Why would s.pskExtension be set in this case? (Or, why do we need the or) Where is it set and what is it set to? |
Lines 54 to 64 in df6e4c8
That said, could just make |
Have you also tested the psk examples to make sure they all still work? Let's check this with 3andne first, to make sure we don't miss any part. |
Yea lets wait for @3andne because I can envision some issues with setting it to lets wait and see |
IMHO
|
Hey @VeNoMouS , can you provide a minimal code snippet that can reproduce the behavior. I need your help on the context. Thanks! |
just use the raw bytes in the first comment and use |
There appears to be a bug/typo, which I'll clarify. In short, the bug is in newSessionController, where we should set these to nil: Lines 57 to 58 in df6e4c8
The But this posed a challenge. The ClientHelloSpec might have a different set of PSK/Session extensions. For clarity, the original design had the To circumvent this, I pivoted to a strategy where the Spec owns the extensions, ensuring the method signature remains unchanged. However, users should be able to override psk/session extensions before invoking This necessitated the creation of the
Yet, during this overhaul, I overlooked the Lines 271 to 273 in df6e4c8
|
@VeNoMouS No problem! Thanks for your efforts! |
Sorry about the delay in getting a PR sorted... life and all that 😄 |
I noticed while trying to implement / understand implementing a PSK for
UtlsPreSharedKeyExtension
onClientHelloSpec
i ran into a weird problem... so i started looking at full raw bytes from wireshark for a full client hello replication , the PSK is not actually populated and errors out , im not sure if this due to the recent change with fakePSK toUtlsPreSharedKeyExtension
empty psk detected; remove the psk extension for this connection or set OmitEmptyPsk to true to conceal it in utls
1603010200010001fc0303cee3cca75d5c385f42d7389f48cf75535b403d58159fecdfdf8ef276bf97deba20a581ab3a59648aead9f3a8715e523c82bee686636cf76c50226295badba9e2ee0020aaaa130113021303c02bc02fc02cc030cca9cca8c013c014009c009d002f003501000193aaaa0000000500050100000000ff0100010000230000000b00020100000d0012001004030804040105030805050108060601001200000010000e000c02683208687474702f312e310033002b00299a9a000100001d00203fcef7f05620d78f9aee265cee94d71e0ced2e692532c18f7cdb6add0055a22d446900050003026832001b000302000200170000000a000a00089a9a001d00170018002d0002010100000010000e00000b746c732e706565742e7773002b000706fafa03040303caca0001000015002c00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000029009c00770071854aabbb2c615e18010431cde6cd36b8e5184aab74d3f8e663befea450d68633ad4fbdf8d14a1e2790e04a3434806eea98e7a33fe5c2c5e74c44dba77c7b1da3e475b9fd6345eb5ad557002f95f36f771a40736de379b4cfe741d351d4121963f35abbd4b4c1a2c1c25e234ab30cb2b786857cf5e6002120ff98ad8e80d2b6d6d2f3e3ab73a5447afe9b79cb9e143aaac613b671b1910d58
This occurs if
RealPSKResumption
is true or falseThe text was updated successfully, but these errors were encountered: