-
Notifications
You must be signed in to change notification settings - Fork 3
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
Expand OpenVPN data key usage #6
base: master
Are you sure you want to change the base?
Conversation
openvpn-wire-protocol.xml
Outdated
key-id. If a renegotiation fails and is reattempted, the key-id | ||
will not be increased. Only the intial TLS session will use the key-id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit ambiguous. It is unclear what key-id will be used. For example:
- Key id is 0
- Renegotiation attempt! Let's us key id 1.
- Oh no, it failed! So we continue with key id 0
- Retry renegotiation. We don't increment the key id. So is it 0 or 1? 0 because previous reneg failed so we don't increment. But oh no! We probably meant for it to be 1?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this?
key-id. If a renegotiation fails and is reattempted, the key-id | |
will not be increased. Only the intial TLS session will use the key-id | |
key-id. If a renegotiation fails and is reattempted, the key-id | |
from the failed attempt will be reused. Only the intial TLS session will use the key-id |
openvpn-wire-protocol.xml
Outdated
<artwork> | ||
min(handshake_window, renegotiation_time/2) | ||
</artwork> | ||
set by the --hand-window and --renog-sec options, which default to 60 and 3600 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing this is a typo
set by the --hand-window and --renog-sec options, which default to 60 and 3600 | |
set by the --hand-window and --reneg-sec options, which default to 60 and 3600 |
In the default configuration of OpenVPN, the time period of promoting a key from | ||
pending to active is calucated as | ||
<artwork> | ||
min(handshake_window, renegotiation_time/2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if renegotiation_time
is 1? :D Is this then half a second? I ask because I use --reneg-sec 1
in a test setup, and I am curious if sub second values are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the implementation becomes broken and directly uses the new key since 1/2 = 0 in integer math:
From ssl.c:
static int
auth_deferred_expire_window(const struct tls_options *o)
{
int ret = o->handshake_window;
const int r2 = o->renegotiate_seconds / 2;
if (o->renegotiate_seconds && r2 < ret)
{
ret = r2;
}
return ret;
}
I didn't want to include this that are more implementation bugs in corner cases into the RFC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense. Thanks for the code snippet. I'll modify my test setup!
Signed-off-by: Arne Schwabe <[email protected]>
7b1e45d
to
81e5443
Compare
No description provided.