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

github repository structure, etc. #30

Closed
kinichiro opened this issue Jul 30, 2014 · 12 comments
Closed

github repository structure, etc. #30

kinichiro opened this issue Jul 30, 2014 · 12 comments

Comments

@kinichiro
Copy link
Contributor

Hi,
here is my thoughts while look around repository.

  1. place of OS-specific source files
    arc4random_(arch).h and getentropy_(arch).c
    under repository 'openbsd' src/lib/libcrypto/crypto/
    should be placed under repository 'portable' crypto/compat/
    like issetugid_(arch).c
    I feel it is better that OS-specific files keep in 'portable' repository only.
  2. bsd-asprintf.c
    crypto/compat/bsd-asprinf.c isn't needed anymore,
    asprintf.c is there instead.

Thanks.

@busterb
Copy link
Contributor

busterb commented Jul 30, 2014

I fixed the bsd-asprintf.c duplication - thanks, that was an oversight.

Are you planning on contributing some OS-specific arc4random/getentropy bits?

@kinichiro
Copy link
Contributor Author

Hi,

I saw asprintf.c duplication was solved. Thanks.

Yes, I would like to contribute OS-specific bits
if no one provide it and LibreSSL team does not plan it.
I am not sure if I have capability to contribute it
and I am eligible to provide it, though ;-)

I looked over libressl-2.0.3.tar.gz and github repositories
to know what is needed to add OS-specific bits.

It seems that 6 files are needed to MODify or ADD.

  1. MOD (portable) configure.ac.tpl - add OS-specific case.
  2. MOD (portable) crypto/Makefile.am.tpl - add OS-specific compat source files.
  3. MOD (portable) crypto/compat/arc4random.h - add arc4random_hpux.h line.
  4. ADD (openbsd) crypto/compat/arc4random_hpux.h - copy from arc4random_solaris.h
  5. ADD (openbsd) crypto/compat/getentropy_hpux.c - based on getentropy_solaris.c, omit dl_iterate_phdr() and getloadavg().
  6. ADD (portable) crypto/compat/issetugid_hpux.c - how about using pstat_getproc(), pst_flag & PS_CHANGEDPRIV.

Then I noticed that OS-specific elements are
contained in 2 repositories, portable and openbsd.
It seems hard to wrap up OS-specific bits in 2 repos within 1 pull-request.

By the way,
How do you receive OS-specific bits patch ?
There is Haiku support request with patch is in openbsd-bugs ML, like this ?
http://marc.info/?l=openbsd-bugs&m=140651474131660&w=2
Or github pull-request ?
Any other way ?

Thanks.

@bob-beck
Copy link
Contributor

kinichiro:

- ADD (openbsd) crypto/compat/getentropy_hpux.c - based on
getentropy_solaris.c, omit dl_iterate_phdr() and getloadavg().

We have concerns about this on platforms we don't have/support. In a
nutshell I would normally be concerned about people simply
removing things in one of these files to make it compile, and calling it
done. What you have removed there will remove entropy
added to that hash in the fallback case. We would like to make sure the
fallback does as good a job as possible, rather than "just make it
compile". - what really needs to happen in there is someone with access to
hpux who understands how this function works checks
to see what we can reasonably obtain without needing descriptor or other
resources that will add entropy to the hash in these cases.

Especially valuable are things like system calls that get get network or
interface statistics, etc. (for example, the osx file gets these
via sysctl).

That's not to say that such things exist on hpux, but we would like to do
as good a job as possible in these before shipping one.

-Bob

On Thu, Jul 31, 2014 at 1:54 AM, kinichiro inoguchi <
notifications@github.com> wrote:

Hi,

I saw asprintf.c duplication was solved. Thanks.

Yes, I would like to contribute OS-specific bits
if no one provide it and LibreSSL team does not plan it.
I am not sure if I have capability to contribute it
and I am eligible to provide it, though ;-)

I looked over libressl-2.0.3.tar.gz and github repositories
to know what is needed to add OS-specific bits.

It seems that 6 files are needed to MODify or ADD.

  1. MOD (portable) configure.ac.tpl - add OS-specific case.
  2. MOD (portable) crypto/Makefile.am.tpl - add OS-specific compat
    source files.
  3. MOD (portable) crypto/compat/arc4random.h - add arc4random_hpux.h
    line.
  4. ADD (openbsd) crypto/compat/arc4random_hpux.h - copy from
    arc4random_solaris.h
  5. ADD (openbsd) crypto/compat/getentropy_hpux.c - based on
    getentropy_solaris.c, omit dl_iterate_phdr() and getloadavg().
  6. ADD (portable) crypto/compat/issetugid_hpux.c - how about using
    pstat_getproc(), pst_flag & PS_CHANGEDPRIV.

Then I noticed that OS-specific elements are
contained in 2 repositories, portable and openbsd.
It seems hard to wrap up OS-specific bits in 2 repos within 1 pull-request.

By the way,
How do you receive OS-specific bits patch ?
There is Haiku support request with patch is in openbsd-bugs ML, like this
?
http://marc.info/?l=openbsd-bugs&m=140651474131660&w=2
Or github pull-request ?
Any other way ?

Thanks.


Reply to this email directly or view it on GitHub
#30 (comment)
.

@busterb
Copy link
Contributor

busterb commented Aug 2, 2014

Maybe we could keep the new OS patches in a 'staging' git branch on portable so people can work on them in one place, before we merge them into an official release.

That would allow more eyes in the respective OS communities to see them.

@bob-beck
Copy link
Contributor

bob-beck commented Aug 2, 2014

I'm not too sure about that. In OpenBSD at least, we always just work on
HEAD. I'm inclined to do that here.
for the most part, if we commit something to head that can then be brought
back to stable (OPENBSD_5_6) either as diff and apply, or if it's just git,
git cherry-pick usually works fabulously.

On Sat, Aug 2, 2014 at 9:56 AM, busterb notifications@github.com wrote:

Maybe we could keep the new OS patches in a 'staging' git branch on
portable so people can work on them in one place, before we merge them into
an official release.

That would allow more eyes in the respective OS communities to see them.


Reply to this email directly or view it on GitHub
#30 (comment)
.

@kinichiro
Copy link
Contributor Author

bob, thanks for pointing out.

I wrote "omit dl_iterate_phdr() and getloadavg()".
I would like to mean that hpux seems doesn't have these 2 system calls,
and it needs something instead of these 2 to keep quality of entropy.

I couldn't decide which system call is appropriate for this purpose.
Then I thought if OS patch is posted as testbed pull request,
we could have discussion for this issue.

I think if someone contributes the new OS patches for this,
he or she should add enough comment that describes what is based on and what is modified.
That would allow communities to state an opinion or propose alternatives to the OS patches.

I hope we could have some new OS patches that is not supported by LibreSSL yet,
and it is good that we could see them here, github portable repository.

Thanks.

@bob-beck
Copy link
Contributor

bob-beck commented Aug 4, 2014

Hi Kinchiro,

A couple of things:

  1. does dl_iterate_phdr() work with gcc on hpux? (If so we may want to
    consider keeping it and insisting on gcc)

Secondly, in a nutshell, what you need to do to help us do a good job here,
is find some system calls that return things that are likely
subject to rapid change, and relatively unique to the system - the gotcha
is these things need to NOT require additional resources,
like opening a file or file descriptor, to use them. If you have a look at
the OSX version, you'll notice on the mac, we can get a bunch
of stuff with "sysctl" - All the networking statistics and some other good
things are fed into the hash. (we can't do this on linux because
sysctl is deprecated). Start by looking at /usr/include/sys/scall_define.h

  • and reading the man pages for each one you don't know what
    it is - I can't seem to find a copy of this file on the net or I'd have a
    look myself. For example, on solaris, this is how I found out that
    "getloadaverage" still existed on solaris - and didn't need anything to
    call - so I added it to the loop feeding the load average into
    the sha512 hash in the fallback.

I'd like to find a way for us to all have a look at the HP/UX syscalls and
have that discussion about what should be in here before
adding this file to libressl portable.

Note - I'll be away for a little bit so feel free to have this discussion
with others than just me ;)

-Bob

On Mon, Aug 4, 2014 at 12:39 AM, kinichiro inoguchi <
notifications@github.com> wrote:

bob, thanks for pointing out.

I wrote "omit dl_iterate_phdr() and getloadavg()".
I would like to mean that hpux seems doesn't have these 2 system calls,
and it needs something instead of these 2 to keep quality of entropy.

I couldn't decide which system call is appropriate for this purpose.
Then I thought if OS patch is posted as testbed pull request,
we could have discussion for this issue.

I think if someone contributes the new OS patches for this,
he or she should add enough comment that describes what is based on and
what is modified.
That would allow communities to state an opinion or propose alternatives
to the OS patches.

I hope we could have some new OS patches that is not supported by LibreSSL
yet,
and it is good that we could see them here, github portable repository.

Thanks.


Reply to this email directly or view it on GitHub
#30 (comment)
.

@kinichiro
Copy link
Contributor Author

  1. does dl_iterate_phdr() work with gcc on hpux?

Now, I don't have "gcc on hpux" environment, then I can't tell about it.
If I could get an environment, I would like to check it.

I'd like to find a way for us to all have a look at the HP/UX syscalls ...

How about this manpage index ? Section 2 lists system calls.
HP-UX Reference - Clickable Manpage Index for HP-UX 11i v3 (March 2012 Update)

And I think pstat(2) series system calls are similar to sysctl() on OSX.

Thanks.

@busterb
Copy link
Contributor

busterb commented Aug 5, 2014

Let's move HP-UX-specific discussions of getentropy off of this ticket. Regarding the original item 1:

My plan is to move getentropy_* to portable after this week. I don't think that will be a problem, and getentropy is loosely coupled to the rest of the arc4random source.

I'm also thinking of consolidating arc4random_osx/solaris into arc4random_posix and having autoconf detect the prerequisite bits required to build it, while leaving that in cvs. That should reduce redundant implementations for systems that support pthread_mutex/atfork and mmap.

That should make it easier for folks to branch and issue pull requests for improvements to getentropy_* without having to juggle two VCS's.

@kinichiro
Copy link
Contributor Author

I think it is good. Thanks.

@busterb
Copy link
Contributor

busterb commented Dec 6, 2014

Well, it has been more than a week, and I never moved getentropy_* into the portable tree. I think we decided to leave those files there, so those get visibility outside of this one project.

But, if you want to renew work on getting HP/UX support into the tree, open a new PR and we'll take a fresh look. IIRC, the issetugid, fork detection and configuration updates looked fine. If you could split the getentropy file from the rest of those, I think I could look at merging that.

@busterb busterb closed this as completed Dec 6, 2014
@kinichiro
Copy link
Contributor Author

I see.

I'll try to open a new PR for issetugid and configuration, and that will contain below,

  • configure.ac
  • crypto/Makefile.am.tpl
  • crypto/compat/arc4random.h
  • crypto/compat/issetugid_hpux.c

And I also open a new PR for openbsd repo, and that will contain below,

  • src/lib/libcrypto/crypto/arc4random_hpux.h
  • src/lib/libcrypto/crypto/getentropy_hpux.c

I would like to do these in this week.

Thanks.

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

No branches or pull requests

3 participants