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

Implement realm otp, webauthn, webauthn passwordless and bruteforce properties #312

Conversation

TuningYourCode
Copy link
Contributor

@TuningYourCode TuningYourCode commented Jun 11, 2024

This PR implements all webauthn and bruteforce properties on a realm supported by at least Keycloak 22.0.0.

WebAuthn Extra Origins and passwordless WebAuthn properties are not yet supported as these were introduced in later Keycloak versions.

@TuningYourCode
Copy link
Contributor Author

@treydock do you know what i did wrong or if there is some kind of debug possibility?

Properties seem to work in my own builds so i don't get why acceptance tests fail.

@treydock
Copy link
Owner

The Keycloak 24.0.3 failures are the following:

       	Error: Parameter web_authn_policy_authenticator_attachment failed on Keycloak_realm[test]: Invalid value "cross-platform". Valid values are plattform, cross-plattform, not specified. (file: /tmp/apply_manifest_151227864.pp.V1dPa1, line: 2)
     Failure/Error: expect(data['rememberMe']).to eq(true)
       
       expected: true
            got: false
     Failure/Error: expect(names).to eq(['profile'])
       
       expected: ["profile"]
            got: ["role_list", "profile", "acr", "web-origins", "roles", "email"]
     Failure/Error: expect(data['eventsEnabled']).to eq(true)
       
       expected: true
            got: false
     Failure/Error: expect(expected_roles - realm_roles).to eq([])
       
       expected: []
            got: ["other_new_role"]

@treydock
Copy link
Owner

For Keycloak 22.0.0 failures, it looks like something is configured in such a way to cause Keycloak to crash.

@TuningYourCode TuningYourCode force-pushed the implement-webauthn-and-bruteforce-properties branch from 2ac1e12 to 5e1787e Compare June 13, 2024 14:22
@TuningYourCode
Copy link
Contributor Author

Thanks, despite the typo i found the issue. It's related to the old version the tests are running against.

e.g. with KeyCloak 23.0.0 introduced the WebAuthn Extra Origins property (https://www.keycloak.org/docs/23.0.0/release_notes/index.html#webauthn-improvements) which was present in my initial PR. It was working in our setup as we already run KeyCloak version 24.0.5.

So i just dropped the properties which are not supported by the tested KeyCloak versions (i anyway don't need them for now)

@TuningYourCode TuningYourCode force-pushed the implement-webauthn-and-bruteforce-properties branch from 5327112 to 31fdd37 Compare June 13, 2024 16:01
@treydock
Copy link
Owner

You can only add these new properties that work with 24.x when that's the version being tested:

let(:new_properties) do
  if RSpec.configuration.keycloak_version.split('.').first.to_i >= 24
    [
       "key => 'value'",
       "anotherkey => 'anothervalue'",
    ]
  else
    []
  end
end

Then in the Puppet code for acceptance tests can do like:

#{new_properties.join("\n")}

I'd be fine merging with the acceptance test lines for 24.x properties commented out and work later to integrate them or just drop 22.x tests and uncomment them later.

Also need to include new properties in unit tests: https://github.com/treydock/puppet-module-keycloak/blob/master/spec/unit/puppet/type/keycloak_realm_spec.rb

@TuningYourCode TuningYourCode force-pushed the implement-webauthn-and-bruteforce-properties branch 2 times, most recently from a71d859 to 53ae965 Compare June 14, 2024 21:21
@TuningYourCode TuningYourCode force-pushed the implement-webauthn-and-bruteforce-properties branch from 410b807 to 897faab Compare June 17, 2024 13:29
@TuningYourCode TuningYourCode changed the title Implement realm webauthn and bruteforce properties Implement realm otp, webauthn, webauthn passwordless and bruteforce properties Jun 17, 2024
@TuningYourCode TuningYourCode force-pushed the implement-webauthn-and-bruteforce-properties branch 3 times, most recently from de210d9 to 7bbc754 Compare June 17, 2024 15:46
@TuningYourCode
Copy link
Contributor Author

From my point of view this PR should be ready for merging now. This PR only includes settings present in at least KeyCloak 22.0.0.

There might be "breaking changes" if somebody is using custom_properties for properties now implemented in the resource. Not sure if you consider them to be "breaking changes" that this PR has to wait for a major release.

I also created a seperate PR (#313) for raising minimal KeyCloak to version 23.0.7 (which is out of support if not the RedHat build is used - see: https://www.keycloak.org/security.html) which might definitly be a "breaking change" for a major puppet module version.

@treydock treydock merged commit a627fa4 into treydock:master Jun 19, 2024
28 checks passed
@treydock
Copy link
Owner

treydock commented Jun 19, 2024

This will be released as 11.2.0 once Github Actions complete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants