8000 OTP support for binary secrets encoded in Base32 by rhyamada · Pull Request #20795 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

OTP support for binary secrets encoded in Base32 - #20795

Closed
rhyamada wants to merge 1 commit into
keycloak:mainfrom
rhyamada:base32_otp_secret
Closed

OTP support for binary secrets encoded in Base32#20795
rhyamada wants to merge 1 commit into
keycloak:mainfrom
rhyamada:base32_otp_secret

Conversation

@rhyamada
@rhyamada rhyamada commented Jun 5, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

Discussion

Started discussion as #20781

Issues

Closes #11561

Closes #9434

One feature/change per PR

Only affects read of secrets stored with prefix {B32}

One commit per PR

#20795
image

Commit

main...rhyamada 8000 :keycloak:base32_otp_secret

`git commit -m "OTP support for binary secrets encoded in Base32" -m "Closes #11561" -m "Closes https://github.com/keycloak/keycloak/issues/9434"``

No changes to code not directly related to your PR

I think so, there one line of change.

-        byte[] k = key.getBytes();
+        byte[] k = key.startsWith("{B32}") ? Base32.decode(key.substring(5)) : key.getBytes(); // if key has {B32} prefix is Base32 encoded

Includes functional/integration test

Has a functional test

    @Test
    public void testHmacBase32() {
        HmacOTP hmacOTP = new HmacOTP(6, HmacOTP.HMAC_SHA1, 10);
        String decoded = "{B32}JNSVMMTEKZCUGSKJIVGHMNSQOZBDA5JT";
        System.out.println(hmacOTP.generateHOTP(decoded, 0));
        System.out.println(hmacOTP.validateHOTP("550233", decoded, 0));
        Assert.assertEquals(1, hmacOTP.validateHOTP("550233", decoded, 0));
    }
    @Test
    public void testHmacBase32Binary() {
        // Secret from https://github.com/keycloak/keycloak/issues/11561
        // Codes values validated aganist https://totp.app/
        HmacOTP hmacOTP = new HmacOTP(6, HmacOTP.HMAC_SHA1, 30);
        String decoded = "{B32}CDLYAYRJ73ORTU4PUWWATWSYQCP4H2QL";
        String counter = "000000000359675C";
        Assert.assertEquals("754397", hmacOTP.generateOTP(decoded, counter, 6, HmacOTP.HMAC_SHA1));
        String counter2 = "0000000003596781";
        Assert.assertEquals("386679", hmacOTP.generateOTP(decoded, counter2, 6, HmacOTP.HMAC_SHA1));
    }

Includes documentation

Where I put this???

Passing all test

Running tests local
Have been passed with:

Test rewritten

HmacTest,java

Test not rewritten but could be affected

mvn -f testsuite/integration-arquillian/pom.xml clean install -Dtest=LoginTotpTest

Previous PR with a test flanky #20755

@rhyamada
rhyamada requested a review from a team as a code owner June 5, 2023 19:11
@ghost ghost added the flaky-test label Jun 5, 2023
ghost
ghost reviewed Jun 5, 2023
View reviewed changes
@ghost ghost left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
ghost commented Jun 5, 2023
Copy link
Copy Markdown

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.model.session.SessionTimeoutsTest#testOnlineUserClientIdleTimeoutSmallerThanSessionNoRefresh

Keycloak CI - Store Model Tests

java.lang.AssertionError: expected null, but was:<org.keycloak.models.sessions.infinispan.AuthenticatedClientSessionAdapter@19093e0c>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotNull(Assert.java:756)
	at org.junit.Assert.assertNull(Assert.java:738)
	at org.junit.Assert.assertNull(Assert.java:748)
...

Report flaky test

rhyamada
rhyamada commented Jun 6, 2023
View reviewed changes
@rhyamada rhyamada left a comment
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think there no relation between theses code changes with reported flaky test

@rhyamada
rhyamada commented Jun 7, 2023
Copy link
Copy Markdown
Contributor Author

I found created Unit Test at https://github.com/keycloak/keycloak/actions/runs/5180973601/jobs/9336085588?pr=20795#step:4:763

@pedroigor
pedroigor commented Jun 21, 2023
Copy link
Copy Markdown
Contributor

@rhyamada I've sent this #21117.

It is basically adding support to set the encoding format used to store a secret and favors using byte[] to properly read secrets.

If you don't mind, we can close this PR and move discussions to the aforementioned PR.

@rhyamada
rhyamada commented Jun 26, 2023
Copy link
Copy Markdown
Contributor Author

As contempled in #21117 I'm closing this PR

@rhyamada rhyamada closed this Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Non ASCII characters in TOTP secret not supported in 2FA configurations OTP base32 decode improvements

3 participants

@rhyamada @pedroigor @ryspr
0