Skip to content
This repository was archived by the owner on Jun 10, 2024. It is now read-only.

Conversation

@davidradl
Copy link
Member

Signed-off-by: David Radley david_radley@uk.ibm.com

Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
README.md Outdated
## SSL configuration

By default the Egeria React UI uses a keystore.p12 file in ssl, this is a copy of file 'https://github.com/odpi/egeria/blob/master/keystore.p12'. These files should be kept the same. This keystore file allows Egeria to run securely in a demo environment; it is not appropriate for production, which should be appropriately secured.

Copy link
Member

Choose a reason for hiding this comment

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

'kept the same' -> 'kept the same if you are using the provided self-signed certificate'.

'securely in a demo' -> 'simply in a demo/development'

'; it is not ..... secured' -> For production use refer to https://egeria-project.org/open-metadata-implementation/admin-services/docs/user/omag-server-platform-transport-level-security.html which provides more information'

Side note - I'm not clear yet (reviewing code) of your use of this file.

  • keystore stores certificates that an app uses to establish a connection (ie as a client to a backend) or to respond (2 way SSL)
  • truststore stores certificates, or more typically certificate authorities, that are used to validate certificates received from the other party

Ideally you need both configurable.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the code it does seem as if trust store is indeed appropriate as this is the store needed to check for known certificates/certificate authorities when the server you are connecting to presents them. It's probably more commonly used in this context afaik. (we may have it wrong in places in egeria)

Now I don't see where the config is to decide whether the client code here should present a certificate to the server for validation , which would be used for 2 way SSL.

Also in this UI what certificate is presented back to the browser user? (that would normally be in the keystore). it's fine to be the same one by default, but it should be configurable to be different one. Typically you'd also expect configuration to decide which certificate to use since a keystore may hold many. Plus the password to access the keystore.

The only challenge may be technically for 2 way ssl you'd also need a truststore to validate the

Copy link
Member Author

@davidradl davidradl Feb 5, 2021

Choose a reason for hiding this comment

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

'kept the same' -> 'kept the same if you are using the provided self-signed certificate'.

'securely in a demo' -> 'simply in a demo/development'

'; it is not ..... secured' -> For production use refer to https://egeria-project.org/open-metadata-implementation/admin-services/docs/user/omag-server-platform-transport-level-security.html which provides more information'

Side note - I'm not clear yet (reviewing code) of your use of this file.

  • keystore stores certificates that an app uses to establish a connection (ie as a client to a backend) or to respond (2 way SSL)
  • truststore stores certificates, or more typically certificate authorities, that are used to validate certificates received from the other party

Ideally you need both configurable.

Fixed the readme as you suggested.

Copy link
Member Author

Choose a reason for hiding this comment

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

@planetf1 I agree the truststore looks best for server authentication. I will change that for now. It would be good to have both.

if (terms.length > 0) {
console.log("issueUpdate " + url);
// Disabling logging as CodeQL does not like user supplied values being logged.
// console.log("issueUpdate " + url);
Copy link
Member

Choose a reason for hiding this comment

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

not an issue as commented out, but i think the key point for the logging is to sanitize any user input. I've not checked code but assume that is the url in this case

cert,
key,
rejectUnauthorized: false,
pfx: keystore,
Copy link
Member

Choose a reason for hiding this comment

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

I presume this means the certificate must be in PKCS#12 format -- this is the most standard, though java also supports java format. I would suggest adding a note to the readme that the keystore etc files must be in PKCS#12 format.

key,
rejectUnauthorized: false,
pfx: keystore,
passphrase: 'egeria'
Copy link
Member

Choose a reason for hiding this comment

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

hardcoded credential ideally should be parameterized as for production deployment org will want to specify own keystore & truststore passwords

cert,
key,
rejectUnauthorized: false,
pfx: keystore,
Copy link
Member

Choose a reason for hiding this comment

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

ditto ref pfx/password

cert,
key,
rejectUnauthorized: false,
pfx: keystore,
Copy link
Member

Choose a reason for hiding this comment

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

as above

@planetf1
Copy link
Member

planetf1 commented Feb 5, 2021

I think the original focus was for the UI server app -> egeria view service communication, not currently sure of the browser -> UI server app interaction -- I'll try and look through the code and add any further comments

Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
Signed-off-by: David Radley <david_radley@uk.ibm.com>
@davidradl davidradl merged commit aaa22b3 into odpi:main Feb 8, 2021
@davidradl davidradl deleted the git60 branch February 8, 2021 09:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants