bolkedebruin / rdpgw

Remote Desktop Gateway in Go for deploying on Linux/BSD/Kubernetes
Apache License 2.0
732 stars 117 forks source link

OIDC / AzureAD error: "securecookie: the value is too long" #15

Closed sphr2k closed 5 months ago

sphr2k commented 3 years ago

Very interesting project - thought I'd give this a try.

Any idea? Is there a flag to get more debug output?

Thanks :)

bolkedebruin commented 3 years ago

The issue is that Azure OIDC probably returns a lot of information in the idtoken. By default this token gets stored in the cookie. This makes the gateway stateless.

It is is possible to configure a different store for this however I don't think I made this configurable so it would require a patch. And again that would make the gateway require a backend like a filesystem or a database. A better workaround would be to limit what Azure OIDC returns.

sphr2k commented 3 years ago

Thanks for your reply. I don't think it ist possible to configure this in Azure AD, but i'll take a look.

jH- commented 2 years ago

@sphr2k Did you look into a workaround for this issue? Currently stuck in the same scenario.

sphr2k commented 2 years ago

Sorry, no, didn't pursue this. Maybe you could try to put Dex in between?

bolkedebruin commented 2 years ago

@jH- you can now configure (rdpgw from master) the gateway to use a filesystem store instead of a cookie store for sessions. This should remove this issue at the expense of needing to keep clients connected to the same rdpgw instance during the exchange of credentials. This is typically the case with load balancers, but ymmv. See the rdpgw.yaml template on how to configure this. Let me know if this works for you

jH- commented 2 years ago

@bolkedebruin Thank you, I'll have a go at it later this week. Will update with results.

sphr2k commented 2 years ago

@bolkedebruin I tried again w/ Azure AD. Getting the following errors:

Web browser:

securecookie: error - caused by: crypto/aes: invalid key size 33

Server_

2022/08/14 18:40:23 http: superfluous response.WriteHeader call from github.com/bolkedebruin/rdpgw/cmd/rdpgw/api.(*Config).HandleCallback (web.go:126)
2022/08/14 18:41:15 http: superfluous response.WriteHeader call from github.com/bolkedebruin/rdpgw/cmd/rdpgw/api.(*Config).HandleCallback (web.go:126)
sphr2k commented 2 years ago

I also tried with Dex as the IDP and and a minimal config - getting the same error.

Here's the Dex config

issuer: https://dex.tld.com

storage:
  type: sqlite3
  config:
    file: /data/dex.db

web:
  http: 127.0.0.1:5556

telemetry:
  http: 127.0.0.1:5558

grpc:
  addr: 127.0.0.1:5557

staticClients:
  - id: rdpgw
    redirectURIs:
      - 'https://rdpgw.tld.com/callback'
    name: 'RDP Gateway'
    secret: ZXhhbXBsZS1hcHAtc2VjcmV0

connectors:
  - type: mockCallback
    id: mock
    name: Example

enablePasswordDB: true

staticPasswords:
  - email: "some@user.com"
    hash: "$2a$10$2b2cU8CPhOTaGrs1HRQuAueS7JTT5ZHsHSzYiFPm1leZck7Mc8T4W"
    username: "admin"
    userID: "08a8684b-db88-4b73-90a9-3cd1661f5466"
bolkedebruin commented 2 years ago

@sphr2k make sure your keys in the security section are exactly 32 characters (I fixed the readme in master). That should solve your error

KoltesDigital commented 1 year ago

Hi,

I'm having the same (original) error. OIDC provider is Azure AD, Server.SessionStore is file, yet the server logs and responds the same.

I've built the Docker image with the current master, and the logged message references this line: https://github.com/bolkedebruin/rdpgw/blob/236ddb4f9b9fc2b40d28f8aa2339b73ce1b0ae9b/cmd/rdpgw/web/oidc.go#L93

bolkedebruin commented 1 year ago

@KoltesDigital please provide the output from the startup of the server.

KoltesDigital commented 1 year ago

It's actually quite simple:

2022/10/21 17:01:11 No valid `security.paatokenencryptionkey` specified (empty or not 32 characters). Setting to random
2022/10/21 17:01:11 No valid `security.paatokensigningkey` specified (empty or not 32 characters). Setting to random
2022/10/21 17:01:11 No valid `security.usertokenencryptionkey` specified (empty or not 32 characters). Setting to random
2022/10/21 17:01:11 No valid `security.usertokensigningkey` specified (empty or not 32 characters). Setting to random
2022/10/21 17:01:11 No valid `server.sessionkey` specified (empty or not 32 characters). Setting to random
2022/10/21 17:01:11 No valid `server.sessionencryptionkey` specified (empty or not 32 characters). Setting to random
2022/10/21 17:01:11 Filesystem is used as session storage
2022/10/21 17:01:11 Starting remote desktop gateway server
2022/10/21 17:01:11 TLS disabled - rdp gw connections require tls, make sure to have a terminator
2022/10/21 17:01:11 enabling openid extended authentication
2022/10/21 17:01:20 Identity SessionId: 7714c83c-6dc4-44c9-aa58-846acb6fb740, UserName: : Authenticated: false
2022/10/21 17:01:20 Identity SessionId: 7714c83c-6dc4-44c9-aa58-846acb6fb740, UserName: : Authenticated: false
2022/10/21 17:01:20 http: superfluous response.WriteHeader call from github.com/bolkedebruin/rdpgw/cmd/rdpgw/web.(*OIDC).HandleCallback (oidc.go:93)

I've skimmed the code, but I'm not a go person and couldn't find how headers would bet set after the content has begun. I'll gladly try if you give me new commit ids to test.

Note that I serve RDPGW behind Traefik which handles TLS, and for now I have only one node so no need to set shared keys. Besides, on the main README, in the config template, you haven't mentioned all the six keys above. Do they all need to be shared among nodes?

bolkedebruin commented 1 year ago

@KoltesDigital I've updated the master branch to set the maximum size of a session to 8kb. It was restricted to 4kb per the same restrictions as http cookies, I thought it would be higher by default with the filesystem storage provider.

Header is set after content has begon due to the error of securecookie too long.

Keys need to be shared across the nodes otherwise they cannot handle each others sessions in case of failover / load balancing. Note that using the filestore make sessions tied to the node. Typically a load balancer will do the right thing here, but ymmv.

Update: it is now configurable by setting maxsessionlength, this only works for the filestore (or non cookie stores).

KoltesDigital commented 1 year ago

Thanks! I confirm a .rdp file is now generated!

It makes sense for the header.

For the keys, I rephrase my question: do all six keys need to be shared, or only four of them as shown in the README?

I have a request, and I'm not confident about being able to make a PR. I'd like the .rdp to have more fields. In particular, I'm interested in authentication level and enablecredsspsupport because target machine is AD joined but client machine is not, as well as use multimon and others. The config template mentions that the Client key can contain rdp file settings, but actually only some of them are supported in the ClientConfig struct. Could you please consider adding a string array in the config, so that these strings are just appended in the .rdp file? I believe it's better than formalizing every settings key: we know which keys we want in the rdp file, so we just write them, no transform needed.

KoltesDigital commented 1 year ago

Ok I tried. Conversation about that request continues on https://github.com/bolkedebruin/rdpgw/pull/58.