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

Appleid #3959

Merged
merged 7 commits into from Feb 14, 2023
Merged

Appleid #3959

merged 7 commits into from Feb 14, 2023

Conversation

mnestor
Copy link
Contributor

@mnestor mnestor commented Feb 11, 2023

Summary

I do have everything working. Probably not the best way in parts though.

Apple really wants their POST for callback:
Had to add MethodPost to the /oauth2/callback handler.
For csrf needed to change samesite to no. With it set to lax the browser will nuke the session on POST and csrf will always fail.

For documentation update, really not much but I'll get a PR over there too. This just adds an idp_provider of "apple"

idp_provider: "apple"
idp_client_id: blah
idp_client_secret:  blah

For routing, we are limited to just checking based on email.

Related issues

Fixes #3958

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@mnestor mnestor requested a review from a team as a code owner February 11, 2023 22:02
@CLAassistant
Copy link

CLAassistant commented Feb 11, 2023

CLA assistant check
All committers have signed the CLA.

@desimone
Copy link
Contributor

Thanks for the PR @mnestor , we have an apple account. Just need to test / confirm this works on our end.

@coveralls
Copy link

coveralls commented Feb 13, 2023

Coverage Status

Coverage: 63.243% (+0.03%) from 63.215% when pulling 266c281 on mnestor:appleid into d0e7b88 on pomerium:main.

@mnestor
Copy link
Contributor Author

mnestor commented Feb 13, 2023

Hopefully G101 issue is resolved here, trying to lint on my system just bombs out.

chore(deps): bump golang from 1.19.5-buster to 1.20.0-buster (pomerium#3949)

Bumps golang from 1.19.5-buster to 1.20.0-buster.

---
updated-dependencies:
- dependency-name: golang
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Denis Mishin <dmishin@pomerium.com>

implemented correct expiration, refresh and revoke

chore(deps): bump golang from 1.19.5-buster to 1.20.0-buster (pomerium#3949)

Bumps golang from 1.19.5-buster to 1.20.0-buster.

---
updated-dependencies:
- dependency-name: golang
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Denis Mishin <dmishin@pomerium.com>

fixed lint issues and maybe ignored G101
@calebdoxsey
Copy link
Contributor

I confirmed that this works.

I suggest two changes, which we can do in subsequent PRs.

  1. Support using the private key directly and generating the JWT according to apple's documentation: https://developer.apple.com/documentation/sign_in_with_apple/generate_and_validate_tokens (a tedious manual step)
  2. Only disabling CSRF for Apple.

@mnestor
Copy link
Contributor Author

mnestor commented Feb 14, 2023

I had been considering #1 already but was trying to keep the changes down. Should it be done in an either or case? Meaning allow the current way or use the private key?

For #2 I will need to think on as that was my first inclination as well as only allowing post for apple but didn't see a path without... a lot more changes. Could just have been a bit of tunnel vision though.

Should I add a #3 for the Post as well?

internal/identity/oauth/apple/apple.go Outdated Show resolved Hide resolved
internal/identity/oauth/apple/apple.go Outdated Show resolved Hide resolved
internal/identity/oauth/apple/apple.go Outdated Show resolved Hide resolved
internal/identity/oauth/apple/apple.go Outdated Show resolved Hide resolved
internal/identity/oauth/apple/apple.go Outdated Show resolved Hide resolved
internal/identity/oauth/apple/apple.go Outdated Show resolved Hide resolved
mnestor and others added 4 commits February 13, 2023 19:46
Co-authored-by: Caleb Doxsey <caleb@doxsey.net>
Co-authored-by: Caleb Doxsey <caleb@doxsey.net>
Co-authored-by: Caleb Doxsey <caleb@doxsey.net>
Co-authored-by: Caleb Doxsey <caleb@doxsey.net>
@calebdoxsey
Copy link
Contributor

I had been considering #1 already but was trying to keep the changes down. Should it be done in an either or case? Meaning allow the current way or use the private key?

For #2 I will need to think on as that was my first inclination as well as only allowing post for apple but didn't see a path without... a lot more changes. Could just have been a bit of tunnel vision though.

Should I add a #3 for the Post as well?

I can implement the changes I mentioned in a separate PR after we merge this one.

I left comments on some changes. If fixing the lint gets the tests to pass I will merge.

mnestor and others added 2 commits February 13, 2023 19:46
Co-authored-by: Caleb Doxsey <caleb@doxsey.net>
Co-authored-by: Caleb Doxsey <caleb@doxsey.net>
@calebdoxsey
Copy link
Contributor

(and yes, for the client secret I will try to make it work either way, which we can detect fairly easily by whether or not the client secret is a JWT or a PEM file)

@mnestor
Copy link
Contributor Author

mnestor commented Feb 14, 2023

Okay. I was going to try and hammer all those out tomorrow but am rather interested to see your way and I'm still newish to Go. Thanks for fixing my comment failures! Already hit commit on them all.

I am thinking there is something wrong with this but it could just be my environment is small and I couldn't figure out separation of services yet so its all in one except for postgres. The auth session seems to be sending the user back to Apple more frequently than I think it should but that just might be my service restarts.

@calebdoxsey
Copy link
Contributor

I think the token is only valid for 10 minutes? Trying to figure out if there is a way to make it last longer.

@calebdoxsey
Copy link
Contributor

Actually its not the token expiration. I'm not sure why I had to re-login. It doesn't seem to be happening now.

@calebdoxsey calebdoxsey merged commit 1d4474f into pomerium:main Feb 14, 2023
@mnestor mnestor deleted the appleid branch February 15, 2023 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Appleid OIDC support
5 participants