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

support for blackfire.io #211

Merged
merged 3 commits into from Jun 4, 2015
Merged

support for blackfire.io #211

merged 3 commits into from Jun 4, 2015

Conversation

leafnode
Copy link
Contributor

The PR adds support for blackfire.io code analysis service. In the form you can provide server id and token that are necessary for the profiler to work.

@InFog
Copy link
Contributor

InFog commented May 20, 2015

I like the idea. I just don't like receiving sensible data like the token.

@leafnode
Copy link
Contributor Author

Reasonable concern. I'll try to work something out with that.

@Nicofuma
Copy link

Actually the server-id and token aren't very sensible and can be shared.

For reference: http://blog.blackfire.io/credentials.html

@leafnode
Copy link
Contributor Author

Yeah, that's what my understanding was, yet I've started implementing prompt during vagrant provision. I can go that way, but I don't think it's necessary.

@InFog
Copy link
Contributor

InFog commented May 21, 2015

Documentation says: "The token-part especially must be kept secret. If you leaked it by mistake, you can and should regenerate it."

I think a prompt during provision is a good option. Just warn user in UI that this is going to happen.

@Nicofuma
Copy link

When you deploy Blackfire on a server only the server credentials matter and they can be leaked because they are used to say "anyone authorized for this server credential can profile this server". So if your server credentials leaked and if someone uses them on his server he will grant you the authorization of profiling his own server. Nothing else.

Only the client credentials must but kept secret.

@@ -50,5 +50,21 @@
{% endfor %}
</select>
</div>
<div class="field">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a link to https://blackfire.io/account/credentials or something? I think it would be handful

@naxhh
Copy link
Contributor

naxhh commented May 21, 2015

I think we can trust @Nicofuma in this one as him should "know something" about this matter :)

A prompt isn't a great idea because it'll break the automated part.

@leafnode
Copy link
Contributor Author

I've prepared a version with prompt, but due to Vagrant limitations, only "secret" (non-echoing) prompt will work. @InFog, which way you me want to pursue?

https://github.com/leafnode/phansible/tree/blackfire_prompt

@leafnode
Copy link
Contributor Author

@InFog the part of the documentation you've cited relate to client token that is not used here at all.

@leafnode
Copy link
Contributor Author

updated the original PR to reflect @naxhh's remarks.

@InFog
Copy link
Contributor

InFog commented May 21, 2015

If data is not sensible, it's fine for me 👍 :)

@leafnode
Copy link
Contributor Author

Great :-) I've just tested the code again, works as expected.

@naxhh
Copy link
Contributor

naxhh commented May 21, 2015

Looks ok.
I want to have a moment to test it!

@leafnode
Copy link
Contributor Author

Sure thing :)

naxhh added a commit that referenced this pull request Jun 4, 2015
@naxhh naxhh merged commit c612c1a into phansible:master Jun 4, 2015
@naxhh
Copy link
Contributor

naxhh commented Jun 4, 2015

Thanks for this!

@fabpot
Copy link

fabpot commented Jun 4, 2015

Thanks!

@InFog InFog removed the Need review label Jun 5, 2015
@leafnode leafnode deleted the blackfire branch June 9, 2015 06:13
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.

None yet

5 participants