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

Fixed issue #40 and #42 #47

Closed

Conversation

christiangda
Copy link
Contributor

@christiangda christiangda commented May 16, 2021

Issue #, if available:

Description of changes:

Fixed error "FATA[0003] status of http response was 404…" when --sync-method "groups"

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@christiangda christiangda changed the title Fixed issue #40 related to FATA[0003] status of http response was 404… Fixed issue #40 and #42 May 16, 2021
@bfreis
Copy link

bfreis commented May 18, 2021

This still doesn't fix #40 .

The process now completes, but the results are different than they were at v1.0.0-rc8, still in a breaking way — now possibly even more dangerous, as the process completes without any indication that something has gone wrong!

The problem is that the groups created on the AWS side have names that are different than they were on v1.0.0-rc8. Specifically, the names were [email protected], but they are now just some-group-name.

Edit: yup, it's dangerous, but even more so than I initially thought. It seems that it created the new groups (with wrong names) and deleted the old ones. The new ones don't have the same aws accounts assigned to them, so this could potentially cause someone to completely lock themselves out of their AWS account via SSO! Re-running rc8 doesn't fix this immediately because, eve though the with the expected names are re-created ultimately they are different groups than the original ones, so they have different IDs and are don't have mappings to the AWS accounts they should have.

I urge you to write proper automated tests for all this behavior, as it's now silently destroying working configurations.

You can potentially mock both AWS and Google APIs, check what are the calls made with rc8, write that ad the expected calls in tests so that rc8 passes, and then make the fixes pass those tests. Anything different than that means that behavior will have changed and can be pretty devastating.

@christiangda
Copy link
Contributor Author

@bfreis

I fixed 404 problem, the Groups name is not a bug is a behaviour, if you want the c1.0.0-rc8 behaviour you need to use the flag --sync-method "users_groups" as the readme say

Local Usage

git clone https://github.com/awslabs/ssosync.git
cd ssosync/
make go-build
./ssosync --help
A command line tool to enable you to synchronise your Google
Apps (Google Workspace) users to AWS Single Sign-on (AWS SSO)
Complete documentation is available at https://github.com/awslabs/ssosync

Usage:
  ssosync [flags]

Flags:
  -t, --access-token string         AWS SSO SCIM API Access Token
  -d, --debug                       enable verbose / debug logging
  -e, --endpoint string             AWS SSO SCIM API Endpoint
  -u, --google-admin string         Google Workspace admin user email
  -c, --google-credentials string   path to Google Workspace credentials file (default "credentials.json")
  -g, --group-match string          Google Workspace Groups filter query parameter, example: 'name:Admin* email:aws-*', see: https://developers.google.com/admin-sdk/directory/v1/guides/search-groups
  -h, --help                        help for ssosync
      --ignore-groups strings       ignores these Google Workspace groups
      --ignore-users strings        ignores these Google Workspace users
      --include-groups strings      include only these Google Workspace groups, NOTE: only works when --sync-method 'users_groups'
      --log-format string           log format (default "text")
      --log-level string            log level (default "info")
  -s, --sync-method string          Sync method to use (users_groups|groups) (default "groups")
  -m, --user-match string           Google Workspace Users filter query parameter, example: 'name:John* email:admin*', see: https://developers.google.com/admin-sdk/directory/v1/guides/search-users
  -v, --version                     version for ssosync

The function has two behaviour and these are controlled by the --sync-method flag, this behavior could be

  1. groups: (default)
    • The sync procedure work base on Groups, gets the Google Workspace groups and their members, then creates in AWS SSO the users (members of the Google Workspace groups), then the groups and at the end assign the users to their respective groups.
    • This method use: Google Workspace groups name --> AWS SSO groups name
  2. users_groups: (original behavior, previous versions)
    • The sync procedure is simple, gets the Google Workspace users and creates these in AWS SSO Users; then gets Google Workspace groups and creates these in AWS SSO Groups and assigns users to belong to the AWS SSO Groups.
    • This method use: Google Workspace groups email --> AWS SSO groups name

Flags Notes:

NOTES:

  1. Depending on the number of users and groups you have, maybe you can get AWS SSO SCIM API rate limits errors, and more frequently happens if you execute the sync many times in a short time.
  2. Depending on the number of users and groups you have, --debug flag generate too much logs lines in your AWS Lambda function. So test it in locally with the --debug flag enabled and disable it when you use a AWS Lambda function.
  3. --sync-method "Groups" and --sync-method "users_groups" are incompatible, because the first use the Google group name as an AWS group name and the second one use the Google group email, take this into consideration.

@bfreis
Copy link

bfreis commented May 18, 2021

the Groups name is not a bug is a behaviour

This is a breaking change. There's simply no other way to put it. It doesn't belong in rc.8 -> rc.9. If you want to introduce a new, different behavior, it should be gated behind a new parameter, and it should be opt-in.

Imagine if someone changed the behavior of ls and made it work like rm -rf --no-preserve-root /, and said, "Hey, if you want the old behavior, you have to pass ls --old-behavior, otherwise it will silently destroy your environment". This is an exaggeration, obviously, but pretty much represents what rc.9 did, and what you're suggesting as the solution.

@badgerspoke
Copy link

As I've mentioned in #46 (comment) I think we need to resolve these issues/differences so we can all move on. I would argue that as @bfreis states this does not fix #40 however this does fix #42 for me.

Is it possible we could drop the reference to #40 and move this forward for #42 only since it seems ok for those on rc9, and those on rc8 can't use this change anyway? (See my post #46 (comment) for how we can resolve the rc8 vs 9 behaviour change.)

@spunkedy
Copy link

This fixed the 404 group issue for me.

@christiangda
Copy link
Contributor Author

Hi guys, I finished the first version of the new alternative project to this one.

https://github.com/slashdevops/idp-scim-sync is a fresh implementation, it is not a ssosync fork, and the main idea is to keep a "state file" after the sync to avoid unnecessary requests to AWS SSO SCIM API.

this project is composed of different artefacts repository but the most convenient one is the AWS Serverless Application https://serverlessrepo.aws.amazon.com/applications/us-east-1/889836709304/idp-scim-sync

eswidler added a commit to panorama-ed/ssosync that referenced this pull request Aug 29, 2022
eswidler added a commit to panorama-ed/ssosync that referenced this pull request Aug 29, 2022
@ChrisPates
Copy link
Contributor

The code base has moved on a long way from here, At some point --sync-method "users_groups" will probably become a wrapper for the --sync-method "groups" method. To ensure interoperability going forward.

@ChrisPates ChrisPates closed this Mar 21, 2024
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.

6 participants