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

Drop support for RSA key generation #1191

Merged
merged 1 commit into from
Aug 21, 2017

Conversation

umayr
Copy link
Contributor

@umayr umayr commented Jul 13, 2017

After this change notary won't be able to generate RSA keys, although it can import and parse keys if provided externally.

CC: @endophage

@umayr umayr force-pushed the feat/no-rsa-key-generate branch from 5d1301d to 0501284 Compare July 13, 2017 19:21
@dingwilson
Copy link
Contributor

Is there any timeline for when RSA support will be dropped?

@umayr umayr changed the title WIP: Drop support for RSA key generation Drop support for RSA key generation Jul 19, 2017
@endophage
Copy link
Contributor

@dingwilson we have no intent to drop support for using externally provided RSA keys. We are only ending support for internally generating RSA keys.

@@ -194,7 +195,7 @@ func createRepoAndKey(t *testing.T, rootType, tempBaseDir, gun, url string) (*No
tempBaseDir, data.GUN(gun), url, http.DefaultTransport, rec.retriever, trustpinning.TrustPinConfig{})
require.NoError(t, err, "error creating repo: %s", err)

rootPubKey, err := repo.CryptoService.Create(data.CanonicalRootRole, repo.gun, rootType)
rootPubKey, err := testutils.CreateOrAddKey(repo.CryptoService, data.CanonicalRootRole, repo.gun, rootType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the rationale for using the testutil instead of the direct cryptoservice call?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah of course – because the cryptoservice cannot create RSA keys? If that is the exact reason, this is perfectly fine then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riyazdf Exactly.

To add more on this, I tried to add keys to crypto service in case of RSA, while for other sort of keys I created them with crypto service which later adds them internally.

Copy link
Contributor

@riyazdf riyazdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a rebase but code otherwise LGTM

@umayr umayr force-pushed the feat/no-rsa-key-generate branch from 0501284 to 35c3062 Compare July 29, 2017 16:58
@umayr
Copy link
Contributor Author

umayr commented Jul 29, 2017

@riyazdf Rebased and fixed the conflicts. Let me know if there's anything else.

Copy link
Contributor

@riyazdf riyazdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @umayr!

Copy link
Contributor

@endophage endophage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have at least one negative test confirming the cryptoservice errors if asked to create an RSA key.

Also should return a specific error for RSA to distinguish it from being a generically unsupported algo.

@umayr umayr force-pushed the feat/no-rsa-key-generate branch from 35c3062 to bc13ee7 Compare August 9, 2017 09:16
@umayr
Copy link
Contributor Author

umayr commented Aug 9, 2017

@endophage I have added an explicit error message that tells rsa key generation is not supported, I have also added a negative case that verifies this moreover, I have rebased it with latest origin/master. Please let me know if it requires anything else.

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.

4 participants