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

Expand OpenVPN data key usage #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

schwabe
Copy link
Collaborator

@schwabe schwabe commented Sep 30, 2024

No description provided.

Comment on lines +1204 to +1205
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
Copy link

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?!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this?

Suggested change
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

<artwork>
min(handshake_window, renegotiation_time/2)
</artwork>
set by the --hand-window and --renog-sec options, which default to 60 and 3600
Copy link

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

Suggested change
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)
Copy link

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.

Copy link
Collaborator Author

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.

Copy link

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!

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