Add WebAuth{, Passwordwordless} Policy

[klausenbusk]

Fix #355

[svenstaro]

Why is this failing CI? Sadly I can't see the result on CircleCI somehow.

I'd like to note that without this PR, it's really annoying to operate this provider with an active WebAuthn config as every time the realm is modified via this Terraform provider, the WebAuthn config is reset to defaults which is really tricky if you forget about it and quite the security risk too.

[klausenbusk]

@mrparkers CI is green now (I just did a squash of all the commits, but he code didn't change), any chance you could have a look? :)

[mrparkers]

Sorry for the delayed review here. Overall the code looks fine, but I think the schema could potentially be improved by aggregating each of these attributes underneath web_authn_policy and web_authn_passwordless_policy blocks. So it might look something like this:

resource "keycloak_realm" "test" {
  name = "test"

  web_authn_policy {
    rp_entity_name = "keycloak"
  }

  web_authn_passwordless_policy {
    rp_entity_name = "keycloak"
  }
}

This would let you mark attributes such as rp_entity_name as required within the web_authn_policy block.

What do you think about this?

[klausenbusk]

What do you think about this?

Sounds good to me, it is a bit easier to manage.

This would let you mark attributes such as rp_entity_name as required within the web_authn_policy block.

Do we want it to be required?

[mrparkers]

I saw that it was required in the GUI, so that's why I mentioned it. But I've never actually used this feature of Keycloak before, so I'll defer to your judgement about whether or not it should be. The only thing I wanted to mention is that this approach would allow us to have a WebAuthn attribute marked as required if we wanted to.

[klausenbusk]

I saw that it was required in the GUI, so that's why I mentioned it. But I've never actually used this feature of Keycloak before, so I'll defer to your judgement about whether or not it should be.

I think Default: "keycloak", is good enough. Anyways the schema change is done and CI is green.

[mrparkers]

just had a few comments about the schema, but overall it looks good.

I do want to add these attributes to an existing test case, or add some new test cases altogether. if you'd prefer, I can work on this and push it to your branch, or you could give it a shot.

[mrparkers]

in the schema for the data source, none of these attributes should have Optional: true or Default, and all of them should have Computed: true

[klausenbusk]

Like this? Should web_authn_policy have Optional: true?

[mrparkers]

I really hate how the Keycloak API uses this as the nil value. I'm not sure how I feel about setting this as the default within the provider package. However, I don't feel strongly enough about this to force you to change it.

[klausenbusk]

The javascript code check if it is not specfied, so... :/

https://github.com/keycloak/keycloak/blob/e239699b8199dc1997227c9caef15115db9eb8e5/themes/src/main/resources/theme/base/login/webauthn-register.ftl#L52

[klausenbusk]

We could use the WebAuthn defaults though. Ex for attestation_conveyance_preference the default is none, any opinions?

[klausenbusk]

if you'd prefer, I can work on this and push it to your branch

Feel free to take a stab at it :) The schema should be fixed now and CI is green.

[mrparkers]

I should have left this comment previously, but I missed it. What would you think about being more explicit with the rp_* attributes, so they say relying_party_* instead? Since the UI refers to them this way, I think it would make this more clear.

[klausenbusk]

Fine with my. I will change it later.

Done and rebased.

[mrparkers]

LGTM - thanks for the PR!