Skip to content

linux support - #40

Merged
heronhaye merged 13 commits into
masterfrom
surya/CORE-10499/secretservice
Apr 8, 2019
Merged

heronhaye merged 13 commits into
masterfrom
surya/CORE-10499/secretservice

Conversation

@heronhaye

@heronhaye heronhaye commented Mar 25, 2019 •

Copy link
Copy Markdown
Contributor

@heronhaye
heronhaye requested a review from patrickxb April 1, 2019 23:57
@@ -0,0 +1,305 @@
package secretservice

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should probably use

// +build linux

for this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You can use dbus on OSX if you want to, is it ok to leave it off? In the client this package will only be imported under +build linux

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sure.

// variables and methods are not exported or easily accessible.
// Note that this protocol is NOT authenticated, NOT secure against malleation
// and is NOT CCA2-secure. It is only meant to hide the D-Bus messages from any
// system services that may be logging everything.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you add some unit tests for these functions? If a lot of this is borrowed from crypto/ssh, are there tests from that package that we can also borrow?

Comment thread secretservice/secretservice.go Outdated
type authenticationMode string

const AuthenticationPlain authenticationMode = "plain"
const AuthenticationDHIETF1024SHA256AES128CBCPKCS7 authenticationMode = "dh-ietf1024-sha256-aes128-cbc-pkcs7"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this is quite a constant name... AuthenticationEncrypted? AuthenticationDH?


switch mode {
case AuthenticationPlain:
sessionAlgorithmInput = dbus.MakeVariant("")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does AuthenticationPlain mean? Is the secret stored in plaintext? If so, can we not support that mode?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The secret is encrypted with the user's keyring password (configured whenever the user set up their keyring). This is just for encrypting the communication between keybase and the keyring, since it's possible something might be logging dbus message history. That's why it doesn't need to be authenticated. I can still remove it if you think it's a good idea though.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's fine, just making sure we weren't supporting a plaintext storage system.

return new(big.Int).Exp(theirPublic, myPrivate, group.p), nil
}

func RFC2409SecondOakleyGroup() *dhGroup {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

golint is going to complain that RFC2409SecondOakleyGroup is exported but is not returning an exported type.

AESKey []byte
}

func NewService() (*SecretService, error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be great to have tests for this too. I assume they will only work on linux, so add

// +build linux

to the test file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can add tests but I don't think they'll work in CI since the system needs to have a keyring running, is that ok?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok

@heronhaye
heronhaye force-pushed the surya/CORE-10499/secretservice branch from a921bee to db4baae Compare April 3, 2019 15:45
@heronhaye

heronhaye commented Apr 3, 2019 •

Copy link
Copy Markdown
Contributor Author

ok @patrickxb, added tests. keyring integration tests are skipped in CI

@heronhaye
heronhaye merged commit 7f2ef9f into master Apr 8, 2019
@heronhaye
heronhaye deleted the surya/CORE-10499/secretservice branch April 8, 2019 19:41
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.

2 participants