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

StandardScrypt has incorrect key for MemoryCost when creating http request #489

Closed
VictorKeil opened this issue Mar 17, 2022 · 4 comments
Closed

Comments

@VictorKeil
Copy link

Describe your environment

  • Operating System version: macOS Monterey V12.3
  • Firebase SDK version: 4
  • Library version: 4.7.1
  • Firebase Product: auth

Describe the problem

ImportUsers request failed with INVALID_HASH_PARAMETERS. After digging through the code and comparing with the Node SDK I noticed that the key for hash.StandardScrypt.MemoryCost was different between the two. When marshaling the hash config into an http request, the correct key for that field is "cpuMemCost", as opposed to "memoryCost", which is the current value.

Steps to reproduce:

Make an ImportUsers request with hash.StandardScrypt hash config as an option.

Relevant Code:

var client *auth.Client
// Initialize client

config := hash.StandardScrypt{
	MemoryCost:       1024,
	Parallelization:  16,
	BlockSize:        8,
	DerivedKeyLength: 64,
}

_, err := client.ImportUsers(ctx, users, auth.WithHash(config))

Firebase response:

{
  "error": {
    "code": 400,
    "message": "INVALID_HASH_PARAMETER",
    "errors": [
      {
        "message": "INVALID_HASH_PARAMETER",
        "domain": "global",
        "reason": "invalid"
      }
    ]
  }
}

Fix:

Just change the key value returned from hash.StandardScrypt.Config() to "cpuMemCost". Should I just do this with a PR? Your guidelines said to post an issue first. Thanks!

@lahirumaramba
Copy link
Member

Hi @VictorKeil Thank you filing this issue. You are right! It looks like cpuMemCost is used for STANDARD_SCRYPT hashing function.

Hi @prameshj, according to the REST API it looks like memoryCost is used for SCRYPT hashing function and cpuMemCost is used for STANDARD_SCRYPT. I checked the Node.js SDK and it seems like we only expose memoryCost field and copies the value over to cpuMemCost in the implementation if STANDARD_SCRYPT is used [ref]. Do you think it makes sense to do something similar in the Go SDK?

@prameshj
Copy link

prameshj commented Mar 17, 2022

For this specific issue, I think we just need to modify this line from"memoryCost" to "cpuMemoryCost" as Victor pointed out.

memoryCost isn't exposed, from what I can tell. It is hardcoded to 1024. If we do expose it, then makes sense to expose a single parameter and copy it to the correct API field internally, like in node.js. memoryCost is indeed exposed, I looked at the snippets in

MemoryCost: 1024,
and incorrectly mentioned that the field is not exposed.

I think it makes sense to have the single exposed field and internally write them to the specific api request field.

@prameshj
Copy link

/assign @VictorKeil

@prameshj prameshj removed their assignment Mar 28, 2022
pragatimodi added a commit to pragatimodi/firebase-admin-go that referenced this issue Aug 25, 2022
lahirumaramba pushed a commit that referenced this issue Sep 8, 2022
Changing the key value "MemoryCost" returned from hash.StandardScrypt.Config() to "cpuMemCost"
@lahirumaramba
Copy link
Member

This should be now fixed in #508
Thank you.

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