Skip to content
This repository has been archived by the owner on Jan 7, 2023. It is now read-only.

Add required bindings to support openssl in libp2p-tls #6

Merged
merged 4 commits into from
Jun 15, 2020

Conversation

poonai
Copy link

@poonai poonai commented May 23, 2020

  • Binding for next_protos
  • binding to add custom extenstion
  • binding to retrive custom extension value

- add support to retrive custom extension value
- add support to add custom protocol for protocol negotiation

Signed-off-by: Tiger <rbalajis25@gmail.com>
Signed-off-by: Tiger <rbalajis25@gmail.com>
@poonai
Copy link
Author

poonai commented May 26, 2020

Friendly ping @Stebalien

@Stebalien
Copy link
Member

Ah, thanks for the reminder. It looks like something dropped the initial notification.

cert.go Outdated
@@ -331,6 +331,14 @@ func (c *Certificate) AddExtension(nid NID, value string) error {
return nil
}

// AddCustomExtension add custom extenstion to the certificate.
func (c *Certificate) AddCustomExtension(nid NID, value []byte) error {
if int(C.add_custom_ext(c.x, C.int(nid), (*C.char)(C.CBytes(value)), C.int(len(value)))) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we copying value inside the C code? If so, I think we need to either free the copied string after calling this, or call (*C.char)(unsafe.Pointer(&value[0])) instead of C.CBytes.

Note: I'm not an expert in CGO so I really have no idea what's safe and what's not safe.

Copy link
Author

Choose a reason for hiding this comment

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

I've freed the memory

cert.go Outdated
// charToBytes converts c unisgned char to golang bytes
func charToBytes(src *C.uchar, sz int) []byte {
dest := make([]byte, sz)
copy(dest, (*(*[1024]byte)(unsafe.Pointer(src)))[:sz:sz])
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is safe. This could cause us to read outside of a mapped page.

I think we need to call C.GoBytes (and maybe copy it? I'm not sure).

Copy link
Author

Choose a reason for hiding this comment

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

Now, I'm using GoBytes.
Thanks for the suggestion.

Signed-off-by: Tiger <rbalajis25@gmail.com>
@poonai
Copy link
Author

poonai commented Jun 13, 2020

@Stebalien I've addressed your comments

@@ -331,6 +331,16 @@ func (c *Certificate) AddExtension(nid NID, value string) error {
return nil
}

// AddCustomExtension add custom extenstion to the certificate.
func (c *Certificate) AddCustomExtension(nid NID, value []byte) error {
val := (*C.char)(C.CBytes(value))
Copy link
Member

Choose a reason for hiding this comment

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

Looking through the OpenSSL source, I'm not sure if this copy is strictly necessary or if we could just pass a pointer into go memory, but it doesn't hurt.

@Stebalien Stebalien merged commit ab3d2c3 into libp2p:master Jun 15, 2020
@Stebalien
Copy link
Member

Thanks!

@poonai poonai deleted the balaji/tls branch June 16, 2020 04:24
@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
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