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

inwx: wait before generating new TOTP TANs #2084

Merged
merged 3 commits into from
Jan 18, 2024
Merged

inwx: wait before generating new TOTP TANs #2084

merged 3 commits into from
Jan 18, 2024

Conversation

gnoack
Copy link
Contributor

@gnoack gnoack commented Jan 16, 2024

This is a workaround for #1608. INWX forbids to re-use the same TOTP twice, but the INWX DNS provider tries to reauthenticate from scratch on each step.

I believe that this is not easily implementable with the existing Lego DNS provider interface, so to avoid refactoring that interaction, let's just make the INWX provider wait a bit until a new token is available. A new token is available every 30 seconds.

The current workaround is to invoke Lego many more times. Retrying at a higher level is worse than retrying here.

Fixes #1608

@ldez ldez self-requested a review January 16, 2024 23:33
@ldez ldez changed the title inwx: Wait before generating new TOTP TANs inwx: wait before generating new TOTP TANs Jan 16, 2024
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

INWX API is weird.

Maybe we can decrease the 30s by reducing the validity period of the TOTP.

	tan, err := totp.GenerateCodeCustom(d.config.SharedSecret, time.Now(), totp.ValidateOpts{
		Period:    10, // <-- the default is 30s
		Skew:      1,
		Digits:    otp.DigitsSix,
		Algorithm: otp.AlgorithmSHA1,
	})

@gnoack
Copy link
Contributor Author

gnoack commented Jan 17, 2024

This is a good idea, but unfortunately not possible. The period is a system parameter, which the client ("prover") and server ("verifier") must agree on. The TOTP QR code can specify the period, or it will be 30 seconds by default.

More mathematically speaking, the "counter" is derived by dividing the current Unix time in seconds by the period in seconds, rounding down. If we made the period 10 instead of 30, we'd end up with counter values that are three times as high (and generate tokens which are valid roughly at epoch+3*(now-epoch), where epoch is Unix time 0).

So unfortunately, this idea does not work.

(Fun fact on the side, as I've been poking around in the RFC... it seems that by rejecting the second authentication with the same token value, INWX is actually following the RFC recommendation in the last paragraph of RFC 6238, section 5.2.)

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

The fact of not being able to reuse the token is expected because it can be a major security issue.
This problem was my fault when I introduced the TOTP system, sometimes you were focusing on something and completely forgot the obvious 😄

I simplified the implementation and added tests to ease the maintenance.

Thank you for your work on this topic, your fix will benefit many users.

Offtopic: I closed (and reopened) the PR by error because I wanted to write something on another screen but I was still inside this page... (GitHub keyboard shortcuts + multiple screens = mistake)

@ldez ldez enabled auto-merge (squash) January 18, 2024 20:33
gnoack and others added 3 commits January 18, 2024 21:33
This is a workaround for go-acme#1608.  INWX forbids to re-use the same TOTP
twice, but the INWX DNS provider tries to reauthenticate from scratch
on each step.

I believe that this is not easily implementable with the existing Lego
DNS provider interface, so to avoid refactoring that interaction,
let's just make the INWX provider wait a bit until a new token is
available.  A new token is available every 30 seconds.

The current workaround is to invoke Lego many more times.  Retrying at
a higher level is worse than retrying here.

Fixes go-acme#1608

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
@ldez ldez merged commit 9d4c60e into go-acme:master Jan 18, 2024
7 checks passed
return 0 * time.Second
}

endPeriod := d.previousCall.Add(30 * time.Second)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While this works, I suspect we are wasting a few seconds here on average, because d.previousCall is never rooted to be a multiple of 30 seconds after the epoch (Unix time 0)?

It works though and how relevant this is depends a bit on the distance between ajacent TOTP attempts.

Copy link
Member

Choose a reason for hiding this comment

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

You are right, sorry I completely missed that.
Sorry, I will try to fix that.
I will truncate the previous time by 30 seconds.

@gnoack
Copy link
Contributor Author

gnoack commented Jan 18, 2024

Thank you @ldez for merging this - much appreciated, also for writing the tests, which I missed :)

I had a look at your patches and I think they work. I left some minor comments - I think it is slowing the wait time down a few seconds more than the previous patch did, but I'll happily stand corrected if I am wrong here. But it works anyway, and is an improvement over what this DNS provider did before.

@ldez ldez added this to the v4.15 milestone Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

INWX: Issue when using 2FA - Same 2FA code is used twice when Present returns non-error
2 participants