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

Add documentation for crypto-related classes. #32285

Merged
merged 1 commit into from Sep 24, 2019

Conversation

@Faless
Copy link
Contributor

commented Sep 23, 2019

Add documentation for Crypto, CryptoKey, HashingContext, and
X509Certificate.
Add documentation for StreamPeerSSL.accept_peer.

Ref #29871.

@Faless Faless added this to the 3.2 milestone Sep 23, 2019
@Faless Faless requested a review from godotengine/documentation as a code owner Sep 23, 2019
@Faless Faless force-pushed the Faless:crypto/initial_docs branch from aed4468 to 526c4ec Sep 24, 2019
[codeblock]
extends Node

var _crypto = Crypto.new()

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

We don't prefix member variables with _ in the https://docs.godotengine.org/en/latest/getting_started/scripting/gdscript/gdscript_styleguide.html, so it would be best not to do it here either. This would also be consistent with the description of generate_self_signed_certificate.

var _cert = X509Certificate.new()

func _ready():
# Generate new RSA key

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

Add a dot at the end of the sentence (same on line 19, line 21 is good).

@@ -35,6 +56,16 @@
<argument index="3" name="not_after" type="String" default="&quot;20340101000000&quot;">
</argument>
<description>
Generate a self-signed [X509Certificate] from the given [CryptoKey] and [code]issuer_name[/code]. The certificate validity will be defined by [code]not_before[/code] and [code]not_after[/code] (first valid date and last valid date). The [code]issuer_name[/code] must contain at least "CN=" (common name, i.e. the domain name), "O=" (organization, i.e. your company name), "C=" (country, i.e. 2 lettered ISO-3166 code of the country the organization is based in).

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

There should not be empty lines in the XML, each line is a <p>. (It's fine in code blocks which are <pre>.)

var key = crypto.generate_rsa(4096)
# Generate self signed certificate using the given key.
var cert = crypto.generate_self_signed_certificate(key, "CN=example.com,O=A Game Company,C=IT")
[codeblock]

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

Missing /.

@@ -35,6 +56,16 @@
<argument index="3" name="not_after" type="String" default="&quot;20340101000000&quot;">
</argument>
<description>
Generate a self-signed [X509Certificate] from the given [CryptoKey] and [code]issuer_name[/code]. The certificate validity will be defined by [code]not_before[/code] and [code]not_after[/code] (first valid date and last valid date). The [code]issuer_name[/code] must contain at least "CN=" (common name, i.e. the domain name), "O=" (organization, i.e. your company name), "C=" (country, i.e. 2 lettered ISO-3166 code of the country the organization is based in).

A small example to generate and RSA key and X509 self-signed certificate.

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

an RSA and a X509

var crypto = Crypto.new()
# Generate 4096 bits RSA key.
var key = crypto.generate_rsa(4096)
# Generate self signed certificate using the given key.

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

Hyphen in "self-signed".

</brief_description>
<description>
The HashingContext class provides an interface for computing cryptographic hashes over multiple iterations. This is useful for example when computing hashes of big files (so you don't have to load them all in memory), network streams, and data streams in general (so you don't have to hold buffers).
The [enum HashType] enum shows the supported hashing algorithms.

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

Remove newline.

var res = ctx.finish()
# Print the result as hex string and array.
printt(res.hex_encode(), Array(res))
[codeblock]

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

Missing /.


[codeblock]
func hash_file(path):
var CHUNK_SIZE = 1024

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

Should it be const outside the function?

# Start an MD5 context.
ctx.start(HashingContext.HASH_MD5)
# Check that file exists.
if not file.file_exists(path): return

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

Line break after :.

@@ -13,12 +32,14 @@
<argument index="0" name="size" type="int">
</argument>
<description>
Generate a [PoolByteArray] of cryptographically secure random bytes with given [code]size[/code].

This comment has been minimized.

Copy link
@akien-mga

akien-mga Sep 24, 2019

Member

Description should be with third-person verb, so "Generates". Same for all other descriptions.

@mhilbrunner

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

IMO we should add a warning to MD5 to only use it for non-security relevant purposes.

This goes for both the HashingContext.HASH_MD5 enum value and the example (or we can use another hash function in the example):

# Start an MD5 context.
ctx.start(HashingContext.HASH_MD5)
Add documentation for Crypto, CryptoKey, HashingContext, and
X509Certificate.
Add documentation for `StreamPeerSSL.accept_peer`.

Ref #29871.
@Faless Faless force-pushed the Faless:crypto/initial_docs branch from 526c4ec to a20cbf2 Sep 24, 2019
@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

@mhilbrunner yeah, I changed it to SHA256 now.

@akien-mga Thanks for the review, everything should be fixed.

@akien-mga akien-mga merged commit 08f557c into godotengine:master Sep 24, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.